Skip to content

Proof of concept fix for #36#37

Merged
jacob-carlborg merged 1 commit intojacob-carlborg:masterfrom
mihails-strasuns-sociomantic:fix-nested-struct-vars
Feb 19, 2015
Merged

Proof of concept fix for #36#37
jacob-carlborg merged 1 commit intojacob-carlborg:masterfrom
mihails-strasuns-sociomantic:fix-nested-struct-vars

Conversation

@mihails-strasuns-sociomantic
Copy link
Copy Markdown
Contributor

Not checked with a test suite and results in extra empty
new lines placed instead but showcases core idea. @jacob-carlborg would you suggest any better place where to put it in dstep sources?

@jacob-carlborg
Copy link
Copy Markdown
Owner

I would like to take a look myself how the Clang AST looks like before I can determine if this is a good solution.

@mihails-strasuns-sociomantic
Copy link
Copy Markdown
Contributor Author

On related topic : is there any pretty printer of raw C AST representation in dstep sources? I didn't spot anything similar, could be useful for debugging.

@jacob-carlborg
Copy link
Copy Markdown
Owner

No, there isn't.

@jacob-carlborg
Copy link
Copy Markdown
Owner

But Clang itself has an AST dumper: $ clang -Xclang -ast-dump <some file>. Some kind of documentation here.

@mihails-strasuns
Copy link
Copy Markdown
Contributor

Awesome, thanks, that will help a lot

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to handle all this in Record.translate. It's these lines that will write a struct declaration inline. When the definition is already know, don't execute those lines.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand. Lines you have linked get called from writeRecord and are responsible for writing the fields (CXCursor_FieldDecl). Struct declarations get written from the return value of writeRecord (return context.data).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines I've linked above are the lines responsible for translating the actual type declaration. The lines below are for the field declaration.

This is the flow of for translating a struct:

  1. All top level nodes are iterated and translated, here
  2. When a struct is seen, it will start translating that, here
  3. writeRecord is called which will translate the identifier of the struct, here
  4. writeRecord then calls the delegate which will start iterating all the children nodes of the struct, here
  5. When a field is seen it will check if the type is not exposed and has a declaration, here
  6. If the type is not exposed and has a declaration it will translate the declaration of the type here
  7. If the declaration is a struct it will start from the beginning at step 2

This code, at step 6, is there to handle a struct and field in the same declaration, i.e.

struct Foo
{
    struct Bar
    {
        int x;
    } bar;
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks, I have missed step 6 where it went in recursion and was confused by seemingly broken iteration over struct members.

@jacob-carlborg
Copy link
Copy Markdown
Owner

Could you please add a testcase.

@mihails-strasuns-sociomantic
Copy link
Copy Markdown
Contributor Author

Updated.

@mihails-strasuns-sociomantic
Copy link
Copy Markdown
Contributor Author

Hm, I have just spotted that it reorders last two struct declaration in generated binding but can't spot obvious reason to do so, any ideas?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use camel casing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Fixes jacob-carlborg#36

Adds static set that keeps track of already known struct definitions
and uses it to check if nested field struct is of already emitted type.

Adds test cases that shows the original issue.
@jacob-carlborg
Copy link
Copy Markdown
Owner

Looking good.

jacob-carlborg added a commit that referenced this pull request Feb 19, 2015
…truct-vars

Fix #36: Don't output duplicate struct definition.
@jacob-carlborg jacob-carlborg merged commit 6744cd6 into jacob-carlborg:master Feb 19, 2015
@jacob-carlborg
Copy link
Copy Markdown
Owner

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants