Re: [PATCH] D22270: [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko closed this revision. Eugene.Zelenko added a comment. Committed in r275460. Repository: rL LLVM https://reviews.llvm.org/D22270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22270: [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
manmanren accepted this revision. manmanren added a comment. This revision is now accepted and ready to land. LGTM. Thanks, Manman Repository: rL LLVM https://reviews.llvm.org/D22270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22270: [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
spyffe updated this revision to Diff 63894.
spyffe added a comment.
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`
Repository:
rL LLVM
http://reviews.llvm.org/D22270
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/Inputs/anonymous-fields1.cpp
test/ASTMerge/Inputs/anonymous-fields2.cpp
test/ASTMerge/anonymous-fields.cpp
Index: test/ASTMerge/anonymous-fields.cpp
===
--- /dev/null
+++ test/ASTMerge/anonymous-fields.cpp
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/anonymous-fields1.cpp
+// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/anonymous-fields2.cpp
+// RUN: %clang_cc1 -emit-obj -o /dev/null -ast-merge %t.1.ast -ast-merge %t.2.ast %s
+// expected-no-diagnostics
Index: test/ASTMerge/Inputs/anonymous-fields2.cpp
===
--- /dev/null
+++ test/ASTMerge/Inputs/anonymous-fields2.cpp
@@ -0,0 +1,9 @@
+class A {
+public:
+ struct { int foo; } f;
+ struct { int foo; } g;
+};
+
+inline int useA(A ) {
+ return (a.f.foo + a.g.foo);
+}
Index: test/ASTMerge/Inputs/anonymous-fields1.cpp
===
--- /dev/null
+++ test/ASTMerge/Inputs/anonymous-fields1.cpp
@@ -0,0 +1,5 @@
+class A {
+public:
+ struct { int foo; } f;
+ struct { int foo; } g;
+};
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1029,7 +1029,7 @@
/// including the next assigned index (if none of them match). Returns an
/// empty option if the context is not a record, i.e.. if the anonymous
/// struct/union is at namespace or block scope.
-static Optional findAnonymousStructOrUnionIndex(RecordDecl *Anon) {
+static Optional findUntaggedStructOrUnionIndex(RecordDecl *Anon) {
ASTContext = Anon->getASTContext();
QualType AnonTy = Context.getRecordType(Anon);
@@ -1040,13 +1040,29 @@
unsigned Index = 0;
for (const auto *D : Owner->noload_decls()) {
const auto *F = dyn_cast(D);
-if (!F || !F->isAnonymousStructOrUnion())
+if (!F)
continue;
-if (Context.hasSameType(F->getType(), AnonTy))
- break;
+if (F->isAnonymousStructOrUnion()) {
+ if (Context.hasSameType(F->getType(), AnonTy))
+break;
+ ++Index;
+ continue;
+}
-++Index;
+// If the field looks like this:
+// struct { ... } A;
+QualType FieldType = F->getType();
+if (const auto *RecType = dyn_cast(FieldType)) {
+ const RecordDecl *RecDecl = RecType->getDecl();
+ if (RecDecl->getDeclContext() == Owner &&
+ !RecDecl->getIdentifier()) {
+if (Context.hasSameType(FieldType, AnonTy))
+ break;
+++Index;
+continue;
+ }
+}
}
return Index;
@@ -1068,8 +1084,8 @@
if (D1->isAnonymousStructOrUnion() && D2->isAnonymousStructOrUnion()) {
// If both anonymous structs/unions are in a record context, make sure
// they occur in the same location in the context records.
-if (Optional Index1 = findAnonymousStructOrUnionIndex(D1)) {
- if (Optional Index2 = findAnonymousStructOrUnionIndex(D2)) {
+if (Optional Index1 = findUntaggedStructOrUnionIndex(D1)) {
+ if (Optional Index2 = findUntaggedStructOrUnionIndex(D2)) {
if (*Index1 != *Index2)
return false;
}
@@ -2749,9 +2765,9 @@
// If both anonymous structs/unions are in a record context, make sure
// they occur in the same location in the context records.
if (Optional Index1
- = findAnonymousStructOrUnionIndex(D)) {
+ = findUntaggedStructOrUnionIndex(D)) {
if (Optional Index2 =
-findAnonymousStructOrUnionIndex(FoundRecord)) {
+findUntaggedStructOrUnionIndex(FoundRecord)) {
if (*Index1 != *Index2)
continue;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22270: [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
spyffe added a comment. I mean `findUntaggedStructOrUnionIndex` Repository: rL LLVM http://reviews.llvm.org/D22270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22270: [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
spyffe added a comment. 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. Repository: rL LLVM http://reviews.llvm.org/D22270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22270: [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
manmanren added a comment.
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
Comment at: lib/AST/ASTImporter.cpp:1054
@@ +1053,3 @@
+ QualType FieldType = F->getType();
+ if (const RecordType *RecType = dyn_cast(FieldType)) {
+const RecordDecl *RecDecl = RecType->getDecl();
Use const auto * here?
Comment at: lib/AST/ASTImporter.cpp:1058
@@ +1057,3 @@
+!RecDecl->getIdentifier()) {
+ if (Context.hasSameType(FieldType, AnonTy)) {
+break;
Combining the two ifs?
Comment at: lib/AST/ASTImporter.cpp:1062
@@ +1061,3 @@
+}
+ }
+}
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: ...
Comment at: test/ASTMerge/anonymous-fields.cpp:3
@@ +2,3 @@
+// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/anonymous-fields2.cpp
+// RUN: %clang_cc1 -emit-obj -o /dev/null -ast-merge %t.1.ast -ast-merge
%t.2.ast %s
+// expected-no-diagnostics
Does this test fail without the change in ASTImporter.cpp? Should we check the
output of the AST merging?
Repository:
rL LLVM
http://reviews.llvm.org/D22270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22270: [ASTImporter] Properly report the locations of anonymous structs declared as part of named fields
spyffe created this revision.
spyffe added a reviewer: manmanren.
spyffe added a subscriber: cfe-commits.
spyffe set the repository for this revision to rL LLVM.
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.
Repository:
rL LLVM
http://reviews.llvm.org/D22270
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/Inputs/anonymous-fields1.cpp
test/ASTMerge/Inputs/anonymous-fields2.cpp
test/ASTMerge/anonymous-fields.cpp
Index: test/ASTMerge/anonymous-fields.cpp
===
--- /dev/null
+++ test/ASTMerge/anonymous-fields.cpp
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/anonymous-fields1.cpp
+// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/anonymous-fields2.cpp
+// RUN: %clang_cc1 -emit-obj -o /dev/null -ast-merge %t.1.ast -ast-merge
%t.2.ast %s
+// expected-no-diagnostics
Index: test/ASTMerge/Inputs/anonymous-fields2.cpp
===
--- /dev/null
+++ test/ASTMerge/Inputs/anonymous-fields2.cpp
@@ -0,0 +1,9 @@
+class A {
+public:
+ struct { int foo; } f;
+ struct { int foo; } g;
+};
+
+inline int useA(A ) {
+ return (a.f.foo + a.g.foo);
+}
Index: test/ASTMerge/Inputs/anonymous-fields1.cpp
===
--- /dev/null
+++ test/ASTMerge/Inputs/anonymous-fields1.cpp
@@ -0,0 +1,5 @@
+class A {
+public:
+ struct { int foo; } f;
+ struct { int foo; } g;
+};
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1040,11 +1040,27 @@
unsigned Index = 0;
for (const auto *D : Owner->noload_decls()) {
const auto *F = dyn_cast(D);
-if (!F || !F->isAnonymousStructOrUnion())
+if (!F)
continue;
-
-if (Context.hasSameType(F->getType(), AnonTy))
- break;
+
+if (F->isAnonymousStructOrUnion()) {
+ if (Context.hasSameType(F->getType(), AnonTy)) {
+break;
+ }
+} else {
+ // If the field looks like this:
+ // struct { ... } A;
+ QualType FieldType = F->getType();
+ if (const RecordType *RecType = dyn_cast(FieldType)) {
+const RecordDecl *RecDecl = RecType->getDecl();
+if (RecDecl->getDeclContext() == Owner &&
+!RecDecl->getIdentifier()) {
+ if (Context.hasSameType(FieldType, AnonTy)) {
+break;
+ }
+}
+ }
+}
++Index;
}
Index: test/ASTMerge/anonymous-fields.cpp
===
--- /dev/null
+++ test/ASTMerge/anonymous-fields.cpp
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/anonymous-fields1.cpp
+// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/anonymous-fields2.cpp
+// RUN: %clang_cc1 -emit-obj -o /dev/null -ast-merge %t.1.ast -ast-merge %t.2.ast %s
+// expected-no-diagnostics
Index: test/ASTMerge/Inputs/anonymous-fields2.cpp
===
--- /dev/null
+++ test/ASTMerge/Inputs/anonymous-fields2.cpp
@@ -0,0 +1,9 @@
+class A {
+public:
+ struct { int foo; } f;
+ struct { int foo; } g;
+};
+
+inline int useA(A ) {
+ return (a.f.foo + a.g.foo);
+}
Index: test/ASTMerge/Inputs/anonymous-fields1.cpp
===
--- /dev/null
+++ test/ASTMerge/Inputs/anonymous-fields1.cpp
@@ -0,0 +1,5 @@
+class A {
+public:
+ struct { int foo; } f;
+ struct { int foo; } g;
+};
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1040,11 +1040,27 @@
unsigned Index = 0;
for (const auto *D : Owner->noload_decls()) {
const auto *F = dyn_cast(D);
-if (!F || !F->isAnonymousStructOrUnion())
+if (!F)
continue;
-
-if (Context.hasSameType(F->getType(), AnonTy))
- break;
+
+if (F->isAnonymousStructOrUnion()) {
+ if (Context.hasSameType(F->getType(), AnonTy)) {
+break;
+ }
+} else {
+ // If the field looks like this:
+ // struct { ... } A;
+ QualType FieldType = F->getType();
+ if (const RecordType *RecType = dyn_cast(FieldType)) {
+const RecordDecl *RecDecl = RecType->getDecl();
+if (RecDecl->getDeclContext() == Owner &&
+!RecDecl->getIdentifier()) {
+ if (Context.hasSameType(FieldType,
