Auto merge of #799 - emilio:comment-indent, r=fitzgen

codegen: Make comments indentation-aware.

This commit moves comment processing to a central place (well, two, because of
field docs, but that's fine).

Also, it makes comments indentation aware, so multiline comments don't appear
garbled.

Finally, it also fixes an out-of-bounds panic when processing an empty multiline
comment.

Cargo.toml

@@ -13,7 +13,7 @@ name = "bindgen"
 readme = "README.md"
 repository = "https://github.com/servo/rust-bindgen"
 documentation = "https://docs.rs/bindgen"
-version = "0.26.3"
+version = "0.27.0"
 build = "build.rs"
 
 exclude = [

src/codegen/helpers.rs

@@ -30,8 +30,8 @@ pub mod attributes {
         aster::AstBuilder::new().attr().word("inline")
     }
 
-    pub fn doc(comment: &str) -> ast::Attribute {
-        aster::AstBuilder::new().attr().doc(comment)
+    pub fn doc(comment: String) -> ast::Attribute {
+        aster::AstBuilder::new().attr().doc(&*comment)
     }
 
     pub fn link_name(name: &str) -> ast::Attribute {

src/codegen/mod.rs

@@ -11,14 +11,14 @@ use aster::struct_field::StructFieldBuilder;
 use ir::annotations::FieldAccessorKind;
 use ir::comp::{Base, BitfieldUnit, Bitfield, CompInfo, CompKind, Field,
                FieldData, FieldMethods, Method, MethodKind};
+use ir::comment;
 use ir::context::{BindgenContext, ItemId};
 use ir::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault};
 use ir::dot;
 use ir::enum_ty::{Enum, EnumVariant, EnumVariantValue};
 use ir::function::{Abi, Function, FunctionSig};
 use ir::int::IntKind;
-use ir::item::{IsOpaque, Item, ItemAncestors, ItemCanonicalName,
-               ItemCanonicalPath, ItemSet};
+use ir::item::{IsOpaque, Item, ItemCanonicalName, ItemCanonicalPath, ItemSet};
 use ir::item_kind::ItemKind;
 use ir::layout::Layout;
 use ir::module::Module;
@@ -42,23 +42,13 @@ use syntax::ptr::P;
 // Name of type defined in constified enum module
 pub static CONSTIFIED_ENUM_MODULE_REPR_NAME: &'static str = "Type";
 
-fn root_import_depth(ctx: &BindgenContext, item: &Item) -> usize {
-    if !ctx.options().enable_cxx_namespaces {
-        return 0;
-    }
-
-    item.ancestors(ctx)
-        .filter(|id| ctx.resolve_item(*id).is_module())
-        .fold(1, |i, _| i + 1)
-}
-
 fn top_level_path(ctx: &BindgenContext, item: &Item) -> Vec<ast::Ident> {
     let mut path = vec![ctx.rust_ident_raw("self")];
 
     if ctx.options().enable_cxx_namespaces {
         let super_ = ctx.rust_ident_raw("super");
 
-        for _ in 0..root_import_depth(ctx, item) {
+        for _ in 0..item.codegen_depth(ctx) {
             path.push(super_.clone());
         }
     }
@@ -616,10 +606,8 @@ impl CodeGenerator for Type {
                 let rust_name = ctx.rust_ident(&name);
                 let mut typedef = aster::AstBuilder::new().item().pub_();
 
-                if ctx.options().generate_comments {
-                    if let Some(comment) = item.comment() {
-                        typedef = typedef.attr().doc(comment);
-                    }
+                if let Some(comment) = item.comment(ctx) {
+                    typedef = typedef.with_attr(attributes::doc(comment));
                 }
 
                 // We prefer using `pub use` over `pub type` because of:
@@ -839,6 +827,7 @@ trait FieldCodegen<'a> {
     fn codegen<F, M>(&self,
                      ctx: &BindgenContext,
                      fields_should_be_private: bool,
+                     codegen_depth: usize,
                      accessor_kind: FieldAccessorKind,
                      parent: &CompInfo,
                      anon_field_names: &mut AnonFieldNames,
@@ -857,6 +846,7 @@ impl<'a> FieldCodegen<'a> for Field {
     fn codegen<F, M>(&self,
                      ctx: &BindgenContext,
                      fields_should_be_private: bool,
+                     codegen_depth: usize,
                      accessor_kind: FieldAccessorKind,
                      parent: &CompInfo,
                      anon_field_names: &mut AnonFieldNames,
@@ -872,6 +862,7 @@ impl<'a> FieldCodegen<'a> for Field {
             Field::DataMember(ref data) => {
                 data.codegen(ctx,
                              fields_should_be_private,
+                             codegen_depth,
                              accessor_kind,
                              parent,
                              anon_field_names,
@@ -884,6 +875,7 @@ impl<'a> FieldCodegen<'a> for Field {
             Field::Bitfields(ref unit) => {
                 unit.codegen(ctx,
                              fields_should_be_private,
+                             codegen_depth,
                              accessor_kind,
                              parent,
                              anon_field_names,
@@ -903,6 +895,7 @@ impl<'a> FieldCodegen<'a> for FieldData {
     fn codegen<F, M>(&self,
                      ctx: &BindgenContext,
                      fields_should_be_private: bool,
+                     codegen_depth: usize,
                      accessor_kind: FieldAccessorKind,
                      parent: &CompInfo,
                      anon_field_names: &mut AnonFieldNames,
@@ -945,8 +938,9 @@ impl<'a> FieldCodegen<'a> for FieldData {
 
         let mut attrs = vec![];
         if ctx.options().generate_comments {
-            if let Some(comment) = self.comment() {
-                attrs.push(attributes::doc(comment));
+            if let Some(raw_comment) = self.comment() {
+                let comment = comment::preprocess(raw_comment, codegen_depth + 1);
+                attrs.push(attributes::doc(comment))
             }
         }
 
@@ -1153,6 +1147,7 @@ impl<'a> FieldCodegen<'a> for BitfieldUnit {
     fn codegen<F, M>(&self,
                      ctx: &BindgenContext,
                      fields_should_be_private: bool,
+                     codegen_depth: usize,
                      accessor_kind: FieldAccessorKind,
                      parent: &CompInfo,
                      anon_field_names: &mut AnonFieldNames,
@@ -1197,6 +1192,7 @@ impl<'a> FieldCodegen<'a> for BitfieldUnit {
         for bf in self.bitfields() {
             bf.codegen(ctx,
                        fields_should_be_private,
+                       codegen_depth,
                        accessor_kind,
                        parent,
                        anon_field_names,
@@ -1277,6 +1273,7 @@ impl<'a> FieldCodegen<'a> for Bitfield {
     fn codegen<F, M>(&self,
                      ctx: &BindgenContext,
                      _fields_should_be_private: bool,
+                     _codegen_depth: usize,
                      _accessor_kind: FieldAccessorKind,
                      parent: &CompInfo,
                      _anon_field_names: &mut AnonFieldNames,
@@ -1409,10 +1406,8 @@ impl CodeGenerator for CompInfo {
         let mut attributes = vec![];
         let mut needs_clone_impl = false;
         let mut needs_default_impl = false;
-        if ctx.options().generate_comments {
-            if let Some(comment) = item.comment() {
-                attributes.push(attributes::doc(comment));
-            }
+        if let Some(comment) = item.comment(ctx) {
+            attributes.push(attributes::doc(comment));
         }
         if self.packed() {
             attributes.push(attributes::repr_list(&["C", "packed"]));
@@ -1545,9 +1540,11 @@ impl CodeGenerator for CompInfo {
 
         let mut methods = vec![];
         let mut anon_field_names = AnonFieldNames::default();
+        let codegen_depth = item.codegen_depth(ctx);
         for field in self.fields() {
             field.codegen(ctx,
                           fields_should_be_private,
+                          codegen_depth,
                           struct_accessor_kind,
                           self,
                           &mut anon_field_names,
@@ -2367,10 +2364,8 @@ impl CodeGenerator for Enum {
             builder = builder.with_attr(attributes::repr("C"));
         }
 
-        if ctx.options().generate_comments {
-            if let Some(comment) = item.comment() {
-                builder = builder.with_attr(attributes::doc(comment));
-            }
+        if let Some(comment) = item.comment(ctx) {
+            builder = builder.with_attr(attributes::doc(comment));
         }
 
         if !is_constified_enum {
@@ -3069,10 +3064,8 @@ impl CodeGenerator for Function {
 
         let mut attributes = vec![];
 
-        if ctx.options().generate_comments {
-            if let Some(comment) = item.comment() {
-                attributes.push(attributes::doc(comment));
-            }
+        if let Some(comment) = item.comment(ctx) {
+            attributes.push(attributes::doc(comment));
         }
 
         if let Some(mangled) = mangled_name {

src/ir/comment.rs

@@ -1,5 +1,7 @@
 //! Utilities for manipulating C/C++ comments.
 
+use std::iter;
+
 /// The type of a comment.
 #[derive(Debug, PartialEq, Eq)]
 enum Kind {
@@ -10,11 +12,12 @@ enum Kind {
     /// entire block ends with `*/`.
     MultiLine,
 }
+
 /// Preprocesses a C/C++ comment so that it is a valid Rust comment.
-pub fn preprocess(comment: String) -> String {
+pub fn preprocess(comment: &str, indent: usize) -> String {
     match self::kind(&comment) {
-        Some(Kind::SingleLines) => preprocess_single_lines(&comment),
-        Some(Kind::MultiLine) => preprocess_multi_line(&comment),
+        Some(Kind::SingleLines) => preprocess_single_lines(comment, indent),
+        Some(Kind::MultiLine) => preprocess_multi_line(comment, indent),
         None => comment.to_owned(),
     }
 }
@@ -30,38 +33,59 @@ fn kind(comment: &str) -> Option<Kind> {
     }
 }
 
+fn make_indent(indent: usize) -> String {
+    const RUST_INDENTATION: usize = 4;
+
+    iter::repeat(' ').take(indent * RUST_INDENTATION).collect()
+}
+
 /// Preprocesses mulitple single line comments.
 ///
 /// Handles lines starting with both `//` and `///`.
-fn preprocess_single_lines(comment: &str) -> String {
-    assert!(comment.starts_with("//"), "comment is not single line");
+fn preprocess_single_lines(comment: &str, indent: usize) -> String {
+    debug_assert!(comment.starts_with("//"), "comment is not single line");
 
+    let indent = make_indent(indent);
+    let mut is_first = true;
     let lines: Vec<_> = comment.lines()
-                               .map(|l| l.trim_left_matches('/').trim())
-                               .map(|l| format!("/// {}", l).trim().to_owned())
-                               .collect();
+        .map(|l| l.trim_left_matches('/').trim())
+        .map(|l| {
+            let indent = if is_first { "" } else { &*indent };
+            is_first = false;
+            let maybe_space = if l.is_empty() { "" } else { " " };
+            format!("{}///{}{}", indent, maybe_space, l)
+        })
+        .collect();
     lines.join("\n")
 }
 
-fn preprocess_multi_line(comment: &str) -> String {
+fn preprocess_multi_line(comment: &str, indent: usize) -> String {
     let comment = comment.trim_left_matches('/')
-                         .trim_left_matches("*")
-                         .trim_left_matches("!")
+                         .trim_left_matches('*')
+                         .trim_left_matches('!')
                          .trim_right_matches('/')
                          .trim_right_matches('*')
                          .trim();
 
+    let indent = make_indent(indent);
     // Strip any potential `*` characters preceding each line.
+    let mut is_first = true;
     let mut lines: Vec<_> = comment.lines()
         .map(|line| line.trim().trim_left_matches('*').trim())
         .skip_while(|line| line.is_empty()) // Skip the first empty lines.
-        .map(|line| format!("/// {}", line).trim().to_owned())
+        .map(|line| {
+            let indent = if is_first { "" } else { &*indent };
+            is_first = false;
+            let maybe_space = if line.is_empty() { "" } else { " " };
+            format!("{}///{}{}", indent, maybe_space, line)
+        })
         .collect();
 
-    // Remove the trailing `*/`.
-    let last_idx = lines.len() - 1;
-    if lines[last_idx].is_empty() {
-        lines.remove(last_idx);
+    // Remove the trailing line corresponding to the `*/`.
+    let last_line_is_empty = lines.last().map_or(false, |l| l.is_empty());
+
+    if last_line_is_empty {
+        lines.pop();
     }
 
     lines.join("\n")
@@ -79,16 +103,16 @@ mod test {
 
     #[test]
     fn processes_single_lines_correctly() {
-        assert_eq!(preprocess("/// hello".to_owned()), "/// hello");
-        assert_eq!(preprocess("// hello".to_owned()), "/// hello");
+        assert_eq!(preprocess("/// hello", 0), "/// hello");
+        assert_eq!(preprocess("// hello", 0), "/// hello");
     }
 
     #[test]
     fn processes_multi_lines_correctly() {
-        assert_eq!(preprocess("/** hello \n * world \n * foo \n */".to_owned()),
+        assert_eq!(preprocess("/** hello \n * world \n * foo \n */", 0),
                    "/// hello\n/// world\n/// foo");
 
-        assert_eq!(preprocess("/**\nhello\n*world\n*foo\n*/".to_owned()),
+        assert_eq!(preprocess("/**\nhello\n*world\n*foo\n*/", 0),
                    "/// hello\n/// world\n/// foo");
     }
 }

src/ir/comp.rs

@@ -1,7 +1,6 @@
 //! Compound types (unions and structs) in our intermediate representation.
 
 use super::annotations::Annotations;
-use super::comment;
 use super::context::{BindgenContext, ItemId};
 use super::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault};
 use super::dot::DotAttributes;
@@ -1102,7 +1101,7 @@ impl CompInfo {
                                                           Some(potential_id),
                                                           ctx);
 
-                    let comment = cur.raw_comment().map(comment::preprocess);
+                    let comment = cur.raw_comment();
                     let annotations = Annotations::new(&cur);
                     let name = cur.spelling();
                     let is_mutable = cursor.is_mutable_field();

src/ir/enum_ty.rs

@@ -1,6 +1,5 @@
 //! Intermediate representation for C/C++ enumerations.
 
-use super::comment;
 use super::context::{BindgenContext, ItemId};
 use super::item::Item;
 use super::ty::TypeKind;
@@ -114,7 +113,7 @@ impl Enum {
                                 })
                         });
 
-                    let comment = cursor.raw_comment().map(comment::preprocess);
+                    let comment = cursor.raw_comment();
                     variants.push(EnumVariant::new(name,
                                                    comment,
                                                    val,

src/ir/function.rs

@@ -1,6 +1,5 @@
 //! Intermediate representation for C/C++ functions and methods.
 
-use super::comment;
 use super::context::{BindgenContext, ItemId};
 use super::dot::DotAttributes;
 use super::item::Item;
@@ -406,7 +405,7 @@ impl ClangSubItemParser for Function {
             mangled_name = None;
         }
 
-        let comment = cursor.raw_comment().map(comment::preprocess);
+        let comment = cursor.raw_comment();
 
         let function = Self::new(name, mangled_name, sig, comment);
         Ok(ParseResult::New(function, Some(cursor)))