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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
