[PATCH] D62131: [ASTImporter] Remove NonEquivalentDecls from ASTImporter

Fri Jun 28 08:10:25 PDT 2019

martong marked 2 inline comments as done.
martong added a comment.

In D62131#1517634 <https://reviews.llvm.org/D62131#1517634>, @a_sidorin wrote:

> Hi Gabor,
>  Could you provide an example of an import sequence leading to this behavior? It's hard for me to imagine such a situation.

Alexei, thanks for the review again. I don't think I could create an import sequence for this. However, I have experienced this kind of poisonous cache behavior during the debugging of a mysterious false positive structural inequivalency (during the analysis of protobuf).
The following happened: During the analysis we compared two Decls which turned out to be inequivalent, so we cached them. Later during the analysis, however, we added a new node to the redecl chain of one of these Decls which we previously compared. Then another structural equivalent check followed for the two Decls. And this time they should have been considered structurally equivalent, but the cache already contained them as nonequivalent. This resulted in a false positive NameConflict error.

By this change the error had gone, and we did not recognize any analysis slowdown. Remember, we still have a cache, but not a global cache in the ASTImporter object.

================
Comment at: clang/test/ASTMerge/struct/test.c:37
 // CHECK: struct2.c:53:37: note: field 'f' has type 'float' here
-// CHECK: struct1.c:54:8: warning: type 'struct DeepError' has incompatible definitions in different translation units
-// CHECK: struct1.c:56:41: note: field 'Deeper' has type 'struct DeeperError *' here
----------------
a_sidorin wrote:
> It looks strange to me that these warnings are gone.
For me, these warnings seems just like noise, by themselves line 37-40 does not show where is the exact mismatch.
The exact mismatch is shown in line 34-36, those warnings indicate that `DeeperError` has incompatible fields in the different TUs with different types (int vs float). That is the lowest level where the mismatch happens, I don't think we should indicate a warning for all other types which contain the mismatched types.

The same is true below (line 50-52) with the struct and union.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62131/new/

https://reviews.llvm.org/D62131