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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
