Don't print out builtin macro definitions with --emit-clang-ast

[fitzgen]

There are a lot of builtin macros that clutter up our AST dump output, and we never actually care about them.

We should skip printing them in clang::ast_dump.

Basically, if cursor.kind() == CXCursor_MacroDefinition && cursor.location() == "builtin definition", then we should skip dumping this cursor's AST.

Happy to mentor whoever picks this up, and clarify further!

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[tsliang]

@fitzgen I'd be happy to take a look. I'll check out clang::ast_dump tonight and see if I have any questions about what I should do.

[emilio]

Awesome! Let us know if you need any help!

[jdm]

@tsliang Have you made any progress with this?

[tsliang]

@jdm unfortunately not - I've been extremely sick for the last two weeks. I'm going to work on it this weekend.

[tsliang]

@jdm @fitzgen

I'm implementing the skip inside the print_cursor subfunction within ast_dump (is this the right place for it btw?). How would you recommend checking whether a cursor is a builtin definition? SourceLocation::Display outputs the literal "builtin definitions," but is there a way to determine a builtin definition that doesn't rely on checking a string literal?

[emilio]

Hi @tsliang!

First of all, hope you're better now :)

See the fn filter_builtins in src/lib.rs for an example of how we filter builtins during parsing. It'd be ideal to share code for both.

Thanks for working on this! :)

[tsliang]

@emilio thanks for the kind words! I am indeed feeling better :)

Thanks for the tip about fn filter_builtins. I like the idea of sharing this logic, but filter_builtins requires a BindgenContext as a parameter, which I don't have access to in ast_dump. How important is the call to ctx.options().builtins in determining whether a cursor is a builtin definition?

[emilio]

@emilio thanks for the kind words! I am indeed feeling better :)

Glad to hear that! :)

Thanks for the tip about fn filter_builtins. I like the idea of sharing this logic, but filter_builtins requires a BindgenContext as a parameter, which I don't have access to in ast_dump. How important is the call to ctx.options().builtins in determining whether a cursor is a builtin definition?

The context parameter is not important to determine whether it's builtin, that is only used to determine whether to generate builtins, so you can remove that argument, and handle it in the caller.

That way, the current:

if !filter_builtins(ctx, cursor) {
    return Continue;
}

would become something like:

if ctx.options().builtins && cursor.is_builtin() {
    return Continue;
}

Where cursor.is_builtin is the function you need to write and that holds that logic that is actually in filter_builtins. Does seem a reasonable way forward?

Thanks again for taking this!

[tsliang]

@emilio thanks for the guidance! I've written something and I'll make a PR later today :)

[tsliang]

ok, one more question: I've done my code changes, and I ran cargo test in the root directory (unlike the last patch I wrote, no libbindgen directory appeared when I built the code). The first time I ran the tests I got 4 failures, but 4 files were changed during the test run (diff attached). After running cargo test again, there are no failures. Is this expected behavior? Should I commit the changed files?

post-test.txt

[fitzgen]

@tsliang

The libbindgen directory has been gone for a month or so now, when did you branch off of master?

If you check out the latest master and run cargo test, do the tests pass without any diif to the test expectations' bindings?

It might also help to open a PR so we can comment on the code.

[emilio]

On Tue, Mar 07, 2017 at 12:40:42PM -0800, Tai Sassen-Liang wrote: ok, one more question: I've done my code changes, and I ran `cargo test` in the root directory (unlike the last patch I wrote, no `libbindgen` directory appeared when I built the code). The first time I ran the tests I got 4 failures, but 4 files were changed during the test run (diff attached). After running `cargo test` again, there are no failures. Is this expected behavior? Should I commit the changed files?

This happens because you have libclang 3.8 instead of 3.9. You can skip the tests that have differences with --features llvm_stable (though maybe it was renamed not long ago?). You can check what travis is doing looking at .travis.yml. In any case, those changes shouldn't be in the final PR. Thanks again! :)

[tsliang]

PR made!

@fitzgen The libbindgen folder was just what I remember from the last patch I wrote, which was months ago. Btw, what's the best way to keep up to date with changes like this? I feel bad for bugging you guys with silly questions like this.

Thanks a lot for the help and patience, @fitzgen @emilio and @jdm

[fitzgen]

Btw, what's the best way to keep up to date with changes like this? I feel bad for bugging you guys with silly questions like this.

It is not "bugging" us (or at least me) at all :-P

The only way to know is to follow the changes happening, read the emails generated by PRs and comments and everything, which can be a bit much at times.

Really it's no big deal to ask a quick question. That's what we're all here for :)

[tsliang]

You're too kind. If you ever come to Amsterdam I'll bring you appreciation snacks :)

[jdm]

This was fixed by #566.