martong marked an inline comment as done.
martong added inline comments.
================
Comment at: lib/AST/ASTImporter.cpp:2085
}
+ } else {
+ if (!IsStructuralMatch(D, FoundRecord, false))
----------------
balazske wrote:
> martong wrote:
> > a_sidorin wrote:
> > > Is it possible to use the added code for the entire condition `if (auto
> > > *FoundRecord = dyn_cast<RecordDecl>(Found))`, replacing its body? Our
> > > structural matching already knows how to handle unnamed structures, and
> > > the upper code partially duplicates
> > > `IsStructurallyEquivalent(StructuralEquivalenceContext
> > > &Context,RecordDecl *D1, RecordDecl *D2)`. Can we change structural
> > > matching to handle this stuff instead of doing it in ASTNodeImporter?
> > Yes, this is a good point, updated the patch accordingly.
> My idea was to remove this whole `if (!SearchName)` branch, but some
> restructuring of the next conditions may be needed. Setting of `PrevDecl` in
> the `if` branch does not look correct (if `!SearchName` is false it is never
> set, unlike in the old code).
I tried what you mentioned Balazs.
It turned out, we can't just skip the whole `if (!SearchName)` branch. Because
in the below case we would match the first unnamed union with the second,
therefore we will break functionality (and some lit tests). The `continue` at
line 2078 is very important in this edge case. (I agree that this code is very
messy and would be good if we could refactor that to be cleaner, particularly
if we could remove the `SearchName` variable would make it way better.)
```
// Matches
struct Unnamed {
union {
struct {
int i;
} S;
struct {
float i;
} R;
} U;
} x14;
```
The failing lit test and it's output:
```
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:84:5:
warning: type 'struct Unnamed::(anonymous at
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:84:5)'
has incompatible definitions in different translation units
struct {
^
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:85:11:
note: field 'i' has type 'int' here
int i;
^
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:88:13:
note: field 'i' has type 'float' here
float i;
^
```
Repository:
rC Clang
https://reviews.llvm.org/D48773
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
