Repository: google/autocxx (rust)

Issue:
Accept defines
`autocxx-gen` needs to accept `-Dfoo=bar` and `-Dfoo` arguments. `autocxx-build` should do similar.

Pull request description:
Add #defines support when processing C++ headers.
Fixes #302.

Patch:
diff --git a/demo/build.rs b/demo/build.rs
index 25071c60..d93f4b7b 100644
--- a/demo/build.rs
+++ b/demo/build.rs
@@ -17,7 +17,8 @@ fn main() {
     // C++ codegen and the macro codegen appears to be run from different
     // working directories.
     let path = std::path::PathBuf::from("src").canonicalize().unwrap();
-    let mut b = autocxx_build::build("src/main.rs", &[&path]).unwrap();
+    let defs: Vec<String> = Vec::new();
+    let mut b = autocxx_build::build("src/main.rs", &[&path], &defs).unwrap();
     b.flag_if_supported("-std=c++14").compile("autocxx-demo");
 
     println!("cargo:rerun-if-changed=src/main.rs");
diff --git a/engine/src/builder.rs b/engine/src/builder.rs
index c8fa9666..f9ece3ce 100644
--- a/engine/src/builder.rs
+++ b/engine/src/builder.rs
@@ -65,6 +65,7 @@ pub type BuilderResult = Result<BuilderSuccess, BuilderError>;
 pub fn build<P1, I, T>(
     rs_file: P1,
     autocxx_incs: I,
+    definitions: &[impl AsRef<str>],
     dependency_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
 ) -> BuilderResult
 where
@@ -72,7 +73,13 @@ where
     I: IntoIterator<Item = T>,
     T: AsRef<OsStr>,
 {
-    build_to_custom_directory(rs_file, autocxx_incs, None, dependency_recorder)
+    build_to_custom_directory(
+        rs_file,
+        autocxx_incs,
+        definitions,
+        None,
+        dependency_recorder,
+    )
 }
 
 /// Builds successfully, or exits the process displaying a suitable
@@ -80,6 +87,7 @@ where
 pub fn expect_build<P1, I, T>(
     rs_file: P1,
     autocxx_incs: I,
+    definitions: &[impl AsRef<str>],
     dependency_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
 ) -> BuilderSuccess
 where
@@ -87,7 +95,7 @@ where
     I: IntoIterator<Item = T>,
     T: AsRef<OsStr>,
 {
-    build(rs_file, autocxx_incs, dependency_recorder).unwrap_or_else(|err| {
+    build(rs_file, autocxx_incs, definitions, dependency_recorder).unwrap_or_else(|err| {
         let _ = writeln!(io::stderr(), "\n\nautocxx error: {}\n\n", report(err));
         process::exit(1);
     })
@@ -102,6 +110,7 @@ fn report(err: BuilderError) -> String {
 pub(crate) fn build_to_custom_directory<P1, I, T>(
     rs_file: P1,
     autocxx_incs: I,
+    definitions: &[impl AsRef<str>],
     custom_gendir: Option<PathBuf>,
     dependency_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
 ) -> BuilderResult
@@ -134,7 +143,7 @@ where
 
     let mut parsed_file = crate::parse_file(rs_file).map_err(BuilderError::ParseError)?;
     parsed_file
-        .resolve_all(autocxx_inc, dependency_recorder)
+        .resolve_all(autocxx_inc, definitions, dependency_recorder)
         .map_err(BuilderError::ParseError)?;
     build_with_existing_parsed_file(parsed_file, cxxdir, incdir, rsdir)
 }
diff --git a/engine/src/conversion/convert_error.rs b/engine/src/conversion/convert_error.rs
index ad14381e..607a2d8e 100644
--- a/engine/src/conversion/convert_error.rs
+++ b/engine/src/conversion/convert_error.rs
@@ -14,8 +14,6 @@
 
 use std::fmt::Display;
 
-use syn::Ident;
-
 use crate::types::{Namespace, TypeName};
 
 #[derive(Debug, Clone)]
@@ -37,14 +35,6 @@ pub enum ConvertError {
     OpaqueTypeFound,
     StaticData(String),
     InfinitelyRecursiveTypedef(TypeName),
-    UnexpectedUseStatement(Option<Ident>),
-}
-
-fn format_maybe_identifier(id: &Option<Ident>) -> String {
-    match id {
-        Some(id) => id.to_string(),
-        None => "<unknown>".into(),
-    }
 }
 
 impl Display for ConvertError {
@@ -67,7 +57,6 @@ impl Display for ConvertError {
             ConvertError::OpaqueTypeFound => write!(f, "Bindgen generated an opaque type (an empty array) somewhere other than a typedef")?,
             ConvertError::StaticData(ty_desc) => write!(f, "Encountered mutable static data, not yet supported: {}", ty_desc)?,
             ConvertError::InfinitelyRecursiveTypedef(tn) => write!(f, "Encountered typedef to itself - this is a known bindgen bug: {}", tn.to_cpp_name())?,
-            ConvertError::UnexpectedUseStatement(maybe_ident) => write!(f, "Unexpected 'use' statement encountered: {}", format_maybe_identifier(maybe_ident))?,
         }
         Ok(())
     }
@@ -90,7 +79,6 @@ impl ConvertError {
                 | ConvertError::UnsupportedType(..)
                 | ConvertError::StaticData(..)
                 | ConvertError::InfinitelyRecursiveTypedef(..)
-                | ConvertError::UnexpectedUseStatement(..)
         )
     }
 }
diff --git a/engine/src/conversion/parse/parse_bindgen.rs b/engine/src/conversion/parse/parse_bindgen.rs
index 5977a524..7afbb125 100644
--- a/engine/src/conversion/parse/parse_bindgen.rs
+++ b/engine/src/conversion/parse/parse_bindgen.rs
@@ -231,12 +231,7 @@ impl<'a> ParseBindgen<'a> {
                             });
                             break;
                         }
-                        _ => {
-                            return Err(ConvertErrorWithIdent(
-                                ConvertError::UnexpectedUseStatement(segs.into_iter().last()),
-                                None,
-                            ))
-                        }
+                        _ => panic!("Unexpected 'use' syntax encountered"),
                     }
                 }
                 Ok(())
diff --git a/engine/src/lib.rs b/engine/src/lib.rs
index 7ca78440..52ef163b 100644
--- a/engine/src/lib.rs
+++ b/engine/src/lib.rs
@@ -272,7 +272,11 @@ impl IncludeCppEngine {
         )
     }
 
-    fn make_bindgen_builder(&self, inc_dirs: &[PathBuf]) -> bindgen::Builder {
+    fn make_bindgen_builder(
+        &self,
+        inc_dirs: &[PathBuf],
+        definitions: &[impl AsRef<str>],
+    ) -> bindgen::Builder {
         let mut builder = bindgen::builder()
             .clang_args(&["-x", "c++", "-std=c++14", "-DBINDGEN"])
             .derive_copy(false)
@@ -294,6 +298,7 @@ impl IncludeCppEngine {
                 i.to_str().expect("Non-UTF8 content in include path")
             )
         }));
+        builder = builder.clang_args(definitions.iter().map(|d| format!("-D{}", d.as_ref())));
 
         // 3. Passes allowlist and other options to the bindgen::Builder equivalent
         //    to --output-style=cxx --allowlist=<as passed in>
@@ -364,6 +369,7 @@ impl IncludeCppEngine {
     pub fn generate(
         &mut self,
         inc_dirs: Vec<PathBuf>,
+        definitions: &[impl AsRef<str>],
         dep_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
     ) -> Result<()> {
         // If we are in parse only mode, do nothing. This is used for
@@ -379,7 +385,7 @@ impl IncludeCppEngine {
             return Err(Error::NoGenerationRequested);
         }
 
-        let mut builder = self.make_bindgen_builder(&inc_dirs);
+        let mut builder = self.make_bindgen_builder(&inc_dirs, &definitions);
         if let Some(dep_recorder) = dep_recorder {
             builder = builder.parse_callbacks(Box::new(AutocxxParseCallbacks(dep_recorder)));
         }
diff --git a/engine/src/parse_file.rs b/engine/src/parse_file.rs
index f4d3947d..fbe4242a 100644
--- a/engine/src/parse_file.rs
+++ b/engine/src/parse_file.rs
@@ -116,6 +116,7 @@ impl ParsedFile {
     pub fn resolve_all(
         &mut self,
         autocxx_inc: Vec<PathBuf>,
+        definitions: &[impl AsRef<str>],
         dep_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
     ) -> Result<(), ParseError> {
         let inner_dep_recorder: Option<Rc<dyn RebuildDependencyRecorder>> =
@@ -128,8 +129,9 @@ impl ParsedFile {
                     inner_dep_recorder.clone(),
                 ))),
             };
+            let defs: Vec<String> = definitions.iter().map(|d| d.as_ref().to_string()).collect();
             include_cpp
-                .generate(autocxx_inc.clone(), dep_recorder)
+                .generate(autocxx_inc.clone(), &defs, dep_recorder)
                 .map_err(ParseError::AutocxxCodegenError)?
         }
         Ok(())
diff --git a/gen/build/src/lib.rs b/gen/build/src/lib.rs
index efac1387..82b3f1ba 100644
--- a/gen/build/src/lib.rs
+++ b/gen/build/src/lib.rs
@@ -23,7 +23,11 @@ use std::{ffi::OsStr, path::Path};
 /// more from a build.rs file.
 /// You need to provide the Rust file path and the iterator of paths
 /// which should be used as include directories.
-pub fn build<P1, I, T>(rs_file: P1, autocxx_incs: I) -> Result<BuilderBuild, BuilderError>
+pub fn build<P1, I, T>(
+    rs_file: P1,
+    autocxx_incs: I,
+    definitions: &[impl AsRef<str>],
+) -> Result<BuilderBuild, BuilderError>
 where
     P1: AsRef<Path>,
     I: IntoIterator<Item = T>,
@@ -33,6 +37,7 @@ where
     engine_build(
         rs_file,
         autocxx_incs,
+        definitions,
         Some(Box::new(CargoRebuildDependencyRecorder)),
     )
     .map(|r| r.0)
@@ -40,7 +45,11 @@ where
 
 /// Builds successfully, or exits the process displaying a suitable
 /// message.
-pub fn expect_build<P1, I, T>(rs_file: P1, autocxx_incs: I) -> BuilderBuild
+pub fn expect_build<P1, I, T>(
+    rs_file: P1,
+    autocxx_incs: I,
+    definitions: &[impl AsRef<str>],
+) -> BuilderBuild
 where
     P1: AsRef<Path>,
     I: IntoIterator<Item = T>,
@@ -50,6 +59,7 @@ where
     engine_expect_build(
         rs_file,
         autocxx_incs,
+        definitions,
         Some(Box::new(CargoRebuildDependencyRecorder)),
     )
     .0
diff --git a/gen/cmd/src/main.rs b/gen/cmd/src/main.rs
index f392ac25..16a1cf25 100644
--- a/gen/cmd/src/main.rs
+++ b/gen/cmd/src/main.rs
@@ -103,6 +103,16 @@ fn main() {
                 .help("include path")
                 .takes_value(true),
         )
+        .arg(
+            Arg::with_name("define")
+                .short("D")
+                .long("define")
+                .multiple(true)
+                .number_of_values(1)
+                .value_name("DEFINITION")
+                .help("preprocessor macro definition")
+                .takes_value(true),
+        )
         .arg(
             Arg::with_name("cpp-extension")
                 .long("cpp-extension")
@@ -160,11 +170,12 @@ fn main() {
         .unwrap_or_default()
         .map(PathBuf::from)
         .collect();
+    let definitions: Vec<_> = matches.values_of("define").unwrap_or_default().collect();
     // In future, we should provide an option to write a .d file here
     // by passing a callback into the dep_recorder parameter here.
     // https://github.com/google/autocxx/issues/56
     parsed_file
-        .resolve_all(incs, None)
+        .resolve_all(incs, &definitions, None)
         .expect("Unable to resolve macro");
     let outdir: PathBuf = matches.value_of_os("outdir").unwrap().into();
     let desired_number = matches
diff --git a/tools/reduce/src/main.rs b/tools/reduce/src/main.rs
index c733c144..07a8acde 100644
--- a/tools/reduce/src/main.rs
+++ b/tools/reduce/src/main.rs
@@ -51,6 +51,16 @@ fn main() {
                 .help("include path")
                 .takes_value(true),
         )
+        .arg(
+            Arg::with_name("define")
+                .short("D")
+                .long("define")
+                .multiple(true)
+                .number_of_values(1)
+                .value_name("DEFINE")
+                .help("macro definition")
+                .takes_value(true),
+        )
         .arg(
             Arg::with_name("header")
                 .short("h")
@@ -128,11 +138,12 @@ fn do_run(matches: ArgMatches, tmp_dir: &TempDir) -> Result<(), std::io::Error>
         .unwrap_or_default()
         .map(PathBuf::from)
         .collect();
+    let defs: Vec<_> = matches.values_of("define").unwrap_or_default().collect();
     let headers: Vec<_> = matches.values_of("header").unwrap_or_default().collect();
     let listing_path = tmp_dir.path().join("listing.h");
     create_concatenated_header(&headers, &listing_path)?;
     let concat_path = tmp_dir.path().join("concat.h");
-    preprocess(&listing_path, &concat_path, &incs)?;
+    preprocess(&listing_path, &concat_path, &incs, &defs)?;
     let rs_path = tmp_dir.path().join("input.rs");
     let directives: Vec<_> = std::iter::once("#include \"concat.h\"\n".to_string())
         .chain(
@@ -250,6 +261,7 @@ fn preprocess(
     listing_path: &Path,
     preprocess_path: &Path,
     incs: &[PathBuf],
+    defs: &[&str],
 ) -> Result<(), std::io::Error> {
     announce_progress("Preprocessing");
     let mut cmd = Command::new("clang++");
@@ -257,6 +269,9 @@ fn preprocess(
     for inc in incs {
         cmd.arg(format!("-I{}", inc.to_str().unwrap()));
     }
+    for def in defs {
+        cmd.arg(format!("-D{}", def));
+    }
     cmd.arg(listing_path.to_str().unwrap());
     let output = cmd.output().expect("failed to preprocess").stdout;
     let output = std::str::from_utf8(&output).unwrap();


Test patch:
diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs
index 6219f666..d623a3a4 100644
--- a/engine/src/integration_tests.rs
+++ b/engine/src/integration_tests.rs
@@ -114,6 +114,7 @@ fn run_test(
         generate,
         generate_pods,
         None,
+        &[],
     )
     .unwrap()
 }
@@ -126,6 +127,7 @@ fn run_test_ex(
     generate: &[&str],
     generate_pods: &[&str],
     extra_directives: Option<TokenStream>,
+    defines: &[&str],
 ) {
     do_run_test(
         cxx_code,
@@ -134,6 +136,7 @@ fn run_test_ex(
         generate,
         generate_pods,
         extra_directives,
+        defines,
     )
     .unwrap()
 }
@@ -152,6 +155,7 @@ fn run_test_expect_fail(
         generate,
         generate_pods,
         None,
+        &[],
     )
     .expect_err("Unexpected success");
 }
@@ -171,6 +175,7 @@ fn do_run_test(
     generate: &[&str],
     generate_pods: &[&str],
     extra_directives: Option<TokenStream>,
+    definitions: &[&str],
 ) -> Result<(), TestError> {
     // Step 1: Write the C++ header snippet to a temp file
     let tdir = tempdir().unwrap();
@@ -226,6 +231,7 @@ fn do_run_test(
     let build_results = crate::builder::build_to_custom_directory(
         &rs_path,
         &[tdir.path()],
+        &definitions,
         Some(target_dir.clone()),
         None,
     )
@@ -243,6 +249,13 @@ fn do_run_test(
         b.file(cxx_path);
     }
 
+    for d in definitions {
+        let mut it = d.split("=");
+        let k = it.next().unwrap();
+        let v = it.next();
+        b.define(k, v);
+    }
+
     b.out_dir(&target_dir)
         .host(&target)
         .target(&target)
@@ -1975,6 +1988,7 @@ fn test_multiple_classes_with_methods() {
         ],
         &["TrivialStruct", "TrivialClass"],
         None,
+        &[],
     );
 }
 
@@ -3951,6 +3965,7 @@ fn test_issues_217_222() {
         &["GURL"],
         &[],
         Some(quote! { block!("StringPiece") block!("Replacements") }),
+        &[],
     );
 }
 
@@ -4112,8 +4127,7 @@ fn test_issue_264() {
         };
         } // namespace operations_research
     "};
-    let rs = quote! {
-    };
+    let rs = quote! {};
     run_test("", hdr, rs, &["operations_research::Solver"], &[]);
 }
 
@@ -4133,8 +4147,7 @@ fn test_vector_of_pointers() {
         class a {};
         } // namespace operations_research
     "};
-    let rs = quote! {
-    };
+    let rs = quote! {};
     run_test("", hdr, rs, &["operations_research::Solver"], &[]);
 }
 
@@ -4151,9 +4164,22 @@ fn test_pointer_to_pointer() {
         class a {};
         } // namespace operations_research
     "};
+    let rs = quote! {};
+    run_test("", hdr, rs, &["operations_research::Solver"], &[]);
+}
+
+#[test]
+fn test_defines_effective() {
+    let hdr = indoc! {"
+        #include <cstdint>
+        #ifdef FOO
+        inline uint32_t a() { return 4; }
+        #endif
+    "};
     let rs = quote! {
+        ffi::a();
     };
-    run_test("", hdr, rs, &["operations_research::Solver"], &[]);
+    run_test_ex("", hdr, rs, &["a"], &[], None, &["FOO"]);
 }
 
 // Yet to test: