[WIP] Trying to solve issue # 548 #596

basic-bgnr · 2017-03-20T16:16:56Z

I've updated the code to dump commandline flag from builder. Extremely simple test is also included in this commit. I haven't tested the code fully for total compliance but I am looking for feedback on further improvement.

… builder

emilio · 2017-03-20T16:36:43Z

r? @fitzgen

fitzgen

Thanks! This is a good start, but there seem to be a couple misunderstandings. See comments below.

fitzgen · 2017-03-20T17:03:14Z

src/lib.rs

@@ -168,6 +168,198 @@ pub fn builder() -> Builder {
 }

 impl Builder {
+    /// Configure and generate Rust bindings for a C/C++ header.
+    ///
+    /// This is the main entry point to the library.


This is not supposed to be the main entry point to the library. This is just a utility for debugging what was originally build.rs usage with command line flags instead.

For example, Stylo uses the Builder API in build.rs, but it is easiest to run creduce with the command line API. So when I am debugging a Stylo + bindgen bug, the first thing I do is try and convert the Builder API usage into equivalent command line flags and arguments so I can use creduce on it.

Nothing here should be mutating a Builder, and it doesn't seem like the builder_ref is mutated so it shouldn't be necessary to take an &mut reference.

Here is the type signature skeleton I would expect:

impl Builder { /// For debugging purposes, dump the set of command line flags that is /// equivalent to this builder invocation. pub fn dump_command_line_flags(&self) -> Vec<String> { // ... } }

fitzgen · 2017-03-20T17:04:27Z

src/lib.rs

+        let mut output_vector:Vec<String> = Vec::new();
+
+        if let Some(ref header) = builder_ref.options.input_header{
+            output_vector.push("header".into());


The input header is a positional argument, and does not have a --header flag.

fitzgen · 2017-03-20T17:06:24Z

src/lib.rs

+
+        if builder_ref.options.bitfield_enums.items.len() > 0 {
+            output_vector.push("bitfield-enum".into());
+            output_vector.extend(builder_ref.options.bitfield_enums.items.iter().cloned());


The --bitfield-enum flag should be in front of every bitfield enum:

for bitfield_enum in self.options.bitfield_nums.items() { output_vector.push("--bitfield-enum".into()); output_vector.push(bitfield_enum.clone()); }

This same fix needs to happen for basically all the flags below.

fitzgen · 2017-03-20T17:06:57Z

src/lib.rs

+
+
+        if builder_ref.options.bitfield_enums.items.len() > 0 {
+            output_vector.push("bitfield-enum".into());


Don't forget to add the "--" prefix to the flags.

fitzgen · 2017-03-20T17:09:12Z

src/regex_set.rs

-    items: Vec<String>,
-    set: Option<RxSet>,
+    pub items: Vec<String>,
+    pub set: Option<RxSet>,


By making these pub, we allow mutation, which we don't want. Instead, let's add getters that return immutable references: &[String] and Option<&RxSet> respectively.

basic-bgnr · 2017-03-20T17:40:27Z

Thank you for the input.
I'll try to answer the listed issue one by one.

Regarding the doc comment, /// Configure and generate Rust bindings for a C/C++ header.: While building with cargo, I found that, comment for each function are required, so i simply copied the comment from previous function to quickly get to build, later i forgot to remove the top line, :-). Sorry for that. I will replace that with /// Generate command line flag for builder.
Regarding the type signature of fn generate_command_line_flag : When you assigned me the issue you described the function as Builder::command_line_flags(), so I thought you wanted a static function. I should have asked you earlier. The mut in builder_ref was me trying to outrun the rust compiler. Thank for pointing that out.
Making the field of RegexSet public: This was a bad idea from beginning, I'll implement a getter function.
prefix --: Will get to it immediately. how did I missed it :-(

Are there any other things to look for in my code?
Thanks you.

fitzgen · 2017-03-20T17:51:45Z

Ah, sorry for misleading you into thinking of a static method! My bad.

Nothing else, looking forward to seeing the next iteration of the code :)

fitzgen

Thanks @basic-bgnr! This is really close :)

I have just a couple nitpicks below, and once fixed up, I think this will be ready to land.

fitzgen · 2017-04-10T18:02:07Z

src/lib.rs

+                     output_vector.push("--bitfield-enum".into());
+                     output_vector.push(item.clone());
+                 })
+            .collect::<Vec<()>>();


Rather than collecting as a Vec to perform map's side effects, the Iterator::count method is more idiomatic for forcing side effects. Even better in this case would be a plain for loop:

for item in self.options.bitfield_enums.get_items() { output_vector.push("--bitfield-enum".into()); output_vector.push(item.clone()); }

fitzgen · 2017-04-10T18:03:44Z

src/lib.rs

+                    .map(|&x| x.into())
+                    .collect::<Vec<String>>();
+
+    assert!(test_cases.iter().all(|ref x| command_line_flags.contains(x)) );


Please add another test that exercises a non-default builder. Something with some whitelisting and opaque-ing perhaps?

fitzgen · 2017-04-10T18:04:14Z

src/lib.rs

+        // output_vector.push("verbose");
+
+
+        output_vector


Just one newline for vertical spacing, not multiple new lines.

fitzgen · 2017-04-10T18:04:32Z

src/lib.rs

+
+
+
+        // output_vector.push("verbose");


Left over from debugging? Please remove commented out code.

fitzgen · 2017-04-10T18:05:31Z

src/regex_set.rs

-
+    /// returns reference to field 'items'
+    pub fn get_items(&self) -> &Vec<String> {
+        self.items.as_ref()


This should return a slice:

pub fn get_items(&self) -> &[String] { &self.items }

fitzgen · 2017-04-10T18:06:33Z

src/regex_set.rs

@@ -27,7 +27,14 @@ impl RegexSet {
            self.insert(s)
        }
    }
-
+    /// returns reference to field 'items'


Please capitalize and punctuate comments:

/// Returns a slice of this `RegexSet`'s items.

Not just here, but elsewhere too.

fitzgen · 2017-04-10T18:09:10Z

Thanks again for taking this all the way home! :)

fitzgen · 2017-04-10T18:09:38Z

One last thing: please squash all the commits into a single commit. Thanks!

#597 (comment)

``` warning: found non-foreign-function-safe member in struct marked #[repr(C)]: found zero-size struct in foreign module, consider adding a member to this struct ```

…g for variants.

…as a string to a Builder Currently the Builder API requires input as header files on disk. This allows passing C source directly to clang using the existing UnsavedFile struct.

…ior.

Signed-off-by: Emilio Cobos Álvarez <[email protected]>

This should fix #584.

…ails

Return `None` whenever we can't find a template definition, not only when the template is a builtin.

But only if the type is not a builtin type. If it is a builtin type, then it's expected that we won't have a definition.

This is exercised in other tests, but in a round about fashion. It is nice to have a test that explicitly exercises default type parameters, without any cruft.

This enables one to locally do $ cargo doc and get library level documentation. Without the changes, cargo doesn't know whether to build docs for the binary or library, and so instead it gives an error and stops.

This replaces various `unwrap` calls with `expect` calls that have better diagnostic messages if/when they fail.

This commit ensures that all of the cargo features we have that only exist for CI/testing purposes, and aren't for external consumption, have a "testing_only_" prefix.

This commit defines a new set of assertion macros that are only checked in testing/CI when the `testing_only_extra_assertions` feature is enabled. This makes it so that *users* of bindgen that happen to be making a debug build don't enable all these extra and expensive assertions. Additionally, this removes the `testing_only_assert_no_dangling_items` feature, and runs the assertions that were previously gated on that feature when the new `testing_only_extra_assertions` feature is enabled.

…ommandline flag. add 2 test conditions

basic-bgnr · 2017-04-11T14:22:58Z

@fitzgen I can't figure what additional changes are required, could you point me to the right direction.

fitzgen · 2017-04-11T21:30:26Z

@basic-bgnr great!

As I mentioned in the last review, please rebase and squash into a single commit rather than merge with upstream master. Here is a basic tutorial, if you're unfamiliar: https://davidwalsh.name/squash-commits-git

Thanks!

basic-bgnr · 2017-04-12T02:36:54Z

@fitzgen I had to start fresh on the pull request. The git history was too convoluted to deal with so i deleted my repo and forked it again. I would like to PR from my new fork, is it possible ?

fitzgen · 2017-04-12T18:05:05Z

@basic-bgnr opening a new PR works, or you can just force push to this PR's branch.

fitzgen · 2017-04-13T16:25:09Z

Closing in favor of #630

basic-bgnr added 2 commits March 20, 2017 21:43

solve issue # 548 related to dumping equivalent commanline flags from…

7da0ed2

… builder

Merge branch 'basic-bgnr_cmd_args'

df5eca7

highfive added the S-awaiting-review label Mar 20, 2017

highfive assigned fitzgen Mar 20, 2017

fitzgen requested changes Mar 20, 2017

View reviewed changes

fitzgen requested changes Apr 10, 2017

View reviewed changes

SimonSapin and others added 19 commits April 11, 2017 08:28

Do not derive Default on vtable types

13f1098

#597 (comment)

Make vtables non-zero-size to fix a rustc warning.

81de3b4

``` warning: found non-foreign-function-safe member in struct marked #[repr(C)]: found zero-size struct in foreign module, consider adding a member to this struct ```

ir: Prefer the enum definition instead of the declaration when lookin…

934dea7

…g for variants.

tests: Disable rustfmt tests, for now.

8ccaf8a

Add a Builder::header_contents method to allow passing source code …

34e5bf4

…as a string to a Builder Currently the Builder API requires input as header files on disk. This allows passing C source directly to clang using the existing UnsavedFile struct.

clang: Fix most of the clang 5.0 regressions in our tests.

f6aa429

#571: Allow ast_dump to print builtins; filter builtins locally

c9763bd

options: Allow configuring the generation of constructors.

cb5eedc

ir: Force the D1 destructor symbol even in older libclang versions.

c5a4afa

Add codegen for destructors.

9b95a42

Refactor a bit and do not generate virtual destructors.

b047a73

options: Allow configuring destructors via CLI.

8128aa9

codegen: Rename the destructor methods to "destruct".

05c2b33

codegen: Add integration tests for destructors.

af02ab7

codegen: Simplify method implementations.

637892c

tests: Add negative tests for constructor/destructor generation behav…

f6c6444

…ior.

ir: Add a note about cpp_demangle.

ead4bf6

ir: Handle char in a more cross-platform way when possible.

da58fb5

Signed-off-by: Emilio Cobos Álvarez <[email protected]>

Tidy up handling of char

f4e3abd

fitzgen and others added 16 commits April 11, 2017 08:28

Fall back to opaque types rather than panicking on parse failure

bb4b08b

Add warning when we fall back to opaque blobs

9a573a9

Ignore builtin template when parsing.

b9f0a76

This should fix #584.

Use opaque types rather than continuing when template instantiation f…

e7fb1d5

…ails

Generalize fallibility template instantiation fallibility

272d832

Return `None` whenever we can't find a template definition, not only when the template is a builtin.

Warn when we can't find a template definition

a114dfd

But only if the type is not a builtin type. If it is a builtin type, then it's expected that we won't have a definition.

Add a test explicitly for default type parameters

f7ed153

This is exercised in other tests, but in a round about fashion. It is nice to have a test that explicitly exercises default type parameters, without any cruft.

Don't build documentation for the binary, just the lib

f4aae50

This enables one to locally do $ cargo doc and get library level documentation. Without the changes, cargo doesn't know whether to build docs for the binary or library, and so instead it gives an error and stops.

Provide better diagnostics for assertions in the template analysis

bdb3ffc

This replaces various `unwrap` calls with `expect` calls that have better diagnostic messages if/when they fail.

Clean up testing-only cargo features

d7bf45d

This commit ensures that all of the cargo features we have that only exist for CI/testing purposes, and aren't for external consumption, have a "testing_only_" prefix.

Omit accessor functions for large bitfields

ed6a839

Move check for field size outside loop

4d4d3d6

update dependencies

653c47a

Add change as required for solving issue #548 related to generating c…

fc40893

…ommandline flag. add 2 test conditions

Merge branch 'master' into master

130e0c3

Merge remote-tracking branch 'upsream/master'

5115c34

fitzgen closed this Apr 13, 2017

Manishearth unassigned fitzgen Jul 25, 2017



		if builder_ref.options.bitfield_enums.items.len() > 0 {
		output_vector.push("bitfield-enum".into());

[WIP] Trying to solve issue # 548 #596

[WIP] Trying to solve issue # 548 #596

Uh oh!

Conversation

basic-bgnr commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio commented Mar 20, 2017

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basic-bgnr commented Mar 20, 2017

Uh oh!

fitzgen commented Mar 20, 2017

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzgen commented Apr 10, 2017

Uh oh!

fitzgen commented Apr 10, 2017

Uh oh!

basic-bgnr commented Apr 11, 2017

Uh oh!

fitzgen commented Apr 11, 2017

Uh oh!

basic-bgnr commented Apr 12, 2017

Uh oh!

fitzgen commented Apr 12, 2017

Uh oh!

fitzgen commented Apr 13, 2017

Uh oh!

Uh oh!

basic-bgnr commented Mar 20, 2017 •

edited

Loading