Page MenuHomePhabricator

[ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
ClosedPublic

Authored by spyffe on Jul 12 2016, 10:59 AM.

Details

Reviewers
manmanren
Summary

When importing classes and structs with anonymous structs, it is critical that distinct anonymous structs remain distinct despite having similar layout.
This is already ensured by distinguishing based on their placement in the parent struct, using the function findAnonymousStructOrUnionIndex.
The problem is that this function only handles

class Foo { struct { int a; } }

and not

class Foo { struct { int a; } var; }

Both need to be handled, and this patch fixes that. The test case ensures that this functionality doesn't regress.

Diff Detail

Repository
rL LLVM

Event Timeline

spyffe updated this revision to Diff 63696.Jul 12 2016, 10:59 AM
spyffe retitled this revision from to [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields.
spyffe updated this object.
spyffe added a reviewer: manmanren.
spyffe set the repository for this revision to rL LLVM.
spyffe added a subscriber: cfe-commits.
manmanren edited edge metadata.Jul 12 2016, 1:44 PM

I am not sure if we should handle this inside findAnonymousStructOrUnionIndex. Here is the definition of anonymous structure I found: An unnamed member whose type specifier is a structure specifier with no tag is called an anonymous structure.

Cheers,
Manman

lib/AST/ASTImporter.cpp
1054

Use const auto * here?

1058

Combining the two ifs?

1062

Should we continue here instead of increasing the Index?

Maybe we can reorder here to reduce nesting?
if (F->isAnonymousStructOrUnion() && ...)

break;

if (F->isAnonymousStructOrUnion())

Index++;
continue;

// If the field looks like this: ...

test/ASTMerge/anonymous-fields.cpp
4

Does this test fail without the change in ASTImporter.cpp? Should we check the output of the AST merging?

Thank you very much for your review, Manman! I can implement all your individual fixes, those look fine. In answer to two of your bigger questions, though:

  • I see what you mean about the definition of an anonymous structure. It looks like our structure is an untagged structure, not an anonymous one. That said, this change is safe for all the places that use this function, so it may be appropriate to change the name to findAnonymousStructOrUnionIndex. What do you think?
  • The test case is unfortunately only an approximation to the more complicated behavior that occurs in lldb. The difference is that the testing infrastructure inside Clang does not implement an ExternalASTSource, which LLDB does. As a result, I've used the test case to verify that we don't break parsing by making this change.

I mean findUntaggedStructOrUnionIndex

Thank you very much for your review, Manman! I can implement all your individual fixes, those look fine. In answer to two of your bigger questions, though:

  • I see what you mean about the definition of an anonymous structure. It looks like our structure is an untagged structure, not an anonymous one. That said, this change is safe for all the places that use this function, so it may be appropriate to change the name to findAnonymousStructOrUnionIndex. What do you think?

Sounds good!

  • The test case is unfortunately only an approximation to the more complicated behavior that occurs in lldb. The difference is that the testing infrastructure inside Clang does not implement an ExternalASTSource, which LLDB does. As a result, I've used the test case to verify that we don't break parsing by making this change.

Ok, thanks for the explanation!

Cheers,
Manman

spyffe updated this revision to Diff 63894.Jul 13 2016, 6:16 PM
spyffe edited edge metadata.

Applied Manman's changes:

  • const auto* instead of const RecordType*
  • kept the if's separate because I...
  • ...moved the Index++ and continue in so that we only increment the counter when we're really dealing with something untagged
  • changed the function name to findUntaggedStructOrUnionIndex
manmanren accepted this revision.Jul 14 2016, 9:43 AM
manmanren edited edge metadata.

LGTM.

Thanks,
Manman

This revision is now accepted and ready to land.Jul 14 2016, 9:43 AM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r275460.