[PATCH] D66538: [AST] AST structural equivalence to work internally with pairs.

[Balázs Kéri via Phabricator via cfe-commits]

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

I remember that we had "problems" with nonsense ODR warnings where both declarations are the same. Probably the reason for it was this cache problem. Still the `NonEquivalentDecls` as cache is useful to filter out the non-equivalences that were already encountered to not produce repeated warnings for them.



================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1357
+
+  EXPECT_FALSE(isInNonEqCache(C));
+  EXPECT_FALSE(isInNonEqCache(findDeclPair<CXXRecordDecl>(
----------------
a_sidorin wrote:
> The declarations of C are not equivalent, but they are not placed into the non-equivalence cache. This looks strange to me, could you please explain this behavior?
We can not know which declarations go into the `NonEquivalentDecls` (at least I do not expect it from this "interface"). It is only meaningful to test that there are no wrong (equivalent) values in it. I think users of this function should not rely on what exactly is put into the `NonEquivalentDecls`  because it is "implementation dependent". Currently the first encountered non-equivalence (during the internal checks) is put into it only, that is here the `A` and `B`. It can be a future work to put every encountered non-equivalent declaration into the cache but even that code will find these only until the first non-equivalence is encountered.


================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1399
+      TU, cxxRecordDecl(hasName("A"), unless(isImplicit())))));
+  EXPECT_FALSE(isInNonEqCache(
+      findDeclPair<FunctionDecl>(TU, functionDecl(hasName("x")))));
----------------
a_sidorin wrote:
> We don't have any tests where isInNonEqCache() can return true. Is it expected?
Yes, the specification for `IsEquivalent` should say that it returns if the declaratiopns are equivalent and puts something into the `NonEquivalentDecls` that is not equivalent. (This "something" is the first encountered non-equivalence and for this the warning message may be produced.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66538