[PATCH] D49296: [ASTImporter] Fix import of unnamed structs

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: lib/AST/ASTStructuralEquivalence.cpp:913
 
-  if (D1->isAnonymousStructOrUnion() && D2->isAnonymousStructOrUnion()) {
+  if (!D1->getDeclName() && !D2->getDeclName()) {
     // If both anonymous structs/unions are in a record context, make sure
----------------
balazske wrote:
> The problem was that value of `isAnonymousStructOrUnion` is not (or not yet) set correctly during the import? But `getDeclName` is always correct (if it is a `false` the record is anonymous struct)?
There is a strange thing here: A struct without a name is not necessarily an anonymous struct!
For example the RecordDecl in `struct { int i; float f; } obj;` is not anonymous.
See the docs for `RecordDecl::isAnonymousStructOrUnion`:
```
  /// Whether this is an anonymous struct or union. To be an anonymous
  /// struct or union, it must have been declared without a name and
  /// there must be no objects of this type declared, e.g.,
  /// @code
  ///   union { int i; float f; };
  /// @endcode
  /// is an anonymous union but neither of the following are:
  /// @code
  ///  union X { int i; float f; };
  ///  union { int i; float f; } obj;
  /// @endcode
  bool isAnonymousStructOrUnion() const { return AnonymousStructOrUnion; }
```
This might seem very perplexed, but this is aligned with the C++17 Standard:
```
12.3.1 Anonymous unions
para 3. A union for which objects, pointers, or references are declared is not an anonymous union.
```

Consequently, we should check whether the Decl has a name, `isAnonymousStructOrUnion` is misleading here.


================
Comment at: unittests/AST/ASTImporterTest.cpp:2495
+      FirstDeclMatcher<FieldDecl>().match(ToTU, fieldDecl(hasName("entry1")));
+  auto getRecordDecl = [](FieldDecl *FD) {
+    auto *ET = cast<ElaboratedType>(FD->getType().getTypePtr());
----------------
a_sidorin wrote:
> There is already a similar lambda definitions for `getRecordDecl()` in ObjectsWithUnnamedStructType test. Can we make it a function, like it is done for StructuralEquivalenceContext?
OK, I added a new function template to handle both cases.


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:578
+
+  ASSERT_TRUE(R0);
+  ASSERT_TRUE(R1);
----------------
balazske wrote:
> Is it possible at all that getRecordDecl returns nullptr? It uses cast (nor cast_or_null and not dyn_cast).
Yes, good catch, changed it.


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:581
+  ASSERT_NE(R0, R1);
+  EXPECT_TRUE(testStructuralMatch(R0, R0));
+  EXPECT_TRUE(testStructuralMatch(R1, R1));
----------------
balazske wrote:
> a_sidorin wrote:
> > Do we really want to test the equivalence of decl to itself, not to its imported version?
> I think this is correct: `R0` should be equivalent with itself but not with `R1`.
I think it makes sense, because from the `StructuralEquivalence`'s viewpoint there is no imported version, just two Decls/Types which may or may not have the same ASTContext.


Repository:
  rC Clang

https://reviews.llvm.org/D49296