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

This revision was automatically updated to reflect the committed changes. Closed by commit rL370639: [AST] AST structural equivalence to work internally with pairs. (authored by balazske, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

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

a_sidorin accepted this revision. a_sidorin added a comment. Looks good, thank you for addressing the comments! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66538/new/ https://reviews.llvm.org/D66538

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

martong added a comment. @a_sidorin Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66538/new/ https://reviews.llvm.org/D66538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

martong added a comment. In D66538#1642999 , @balazske wrote: > In D66538#1642883 , @martong wrote: > > > There is a third test which could be useful to test whether there is no > > faulty cache entries there: > >

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

balazske updated this revision to Diff 217105. balazske added a comment. - Small update to newer C++ syntax, use std::pair in test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66538/new/ https://reviews.llvm.org/D66538 Files:

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

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

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

a_sidorin added a comment. Hello Balasz, I have some comments inline. Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1579 + D2 = D2->getCanonicalDecl(); + std::pair P = std::make_pair(D1, D2); + `std::pair P{D1, D2}`? Comment at:

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

balazske added a comment. In D66538#1642883 , @martong wrote: > There is a third test which could be useful to test whether there is no > faulty cache entries there: > > +TEST_F(StructuralEquivalenceCacheTest, DISABLED_NonEq) { > ... > +} > This

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

balazske updated this revision to Diff 216851. balazske added a comment. - Updated tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66538/new/ https://reviews.llvm.org/D66538 Files: clang/include/clang/AST/ASTStructuralEquivalence.h

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

martong added a comment. There is a third test which could be useful to test whether there is no faulty cache entries there: +TEST_F(StructuralEquivalenceCacheTest, DISABLED_NonEq) { + auto Decls = + makeTuDecls( + R"( +class A {}; +class B {

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

martong added a comment. Ok. I like this patch because it eliminates the need for checking the redeclaration chains. Seems like it handles cycles and the simple `f(A,B)` vs `f(A,A)` cases properly too. (Not talking about caching now, probably we must remove the `NonEquivalentDecls` cache.)

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

balazske added a comment. The structural equivalency check works (with this patch) the following way: To check a `(A1,A2)` pair we collect further `(X1,X2)` pairs that should be checked and put these in `DeclsToCheck` (if not already there). The check succeeds if every pair is equivalent. The

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

martong added a comment. The link for the diff went off, sorry about that. Here is the new link which is going to work: https://github.com/martong/clang/compare/NonEqDecls_cache_in_an_upper_level_0...martong:NonEqDecls_cache_in_an_upper_level?expand=1 Repository: rG LLVM Github Monorepo

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

balazske added a comment. In D66538#1641310 , @martong wrote: > > It looks like that the original code is correct in the decision of > > structural equivalence of the original pair. If we have an (A,B) and (A,C) > > to compare, B and C are in different

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

martong added a comment. > It looks like that the original code is correct in the decision of structural > equivalence of the original pair. If we have an (A,B) and (A,C) to compare, B > and C are in different decl chain, then (A,B) or (A,C) will be non-equivalent > (unless B and C are

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

balazske marked an inline comment as done. balazske added inline comments. Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1323 +NonEquivalentDecls.end()); + EXPECT_EQ(NonEquivalentDecls.find(std::make_pair(y1, y2)), +

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

martong added inline comments. Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1323 +NonEquivalentDecls.end()); + EXPECT_EQ(NonEquivalentDecls.find(std::make_pair(y1, y2)), +NonEquivalentDecls.end()); This should be

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

martong added inline comments. Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1586 - if (EquivToD1) -return EquivToD1 == D2->getCanonicalDecl(); balazske wrote: > It looks like that the original code is correct in the decision of structural >

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

balazske marked an inline comment as done. balazske added inline comments. Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1586 - if (EquivToD1) -return EquivToD1 == D2->getCanonicalDecl(); It looks like that the original code is correct in the

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

balazske created this revision. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp. Herald added a reviewer: martong. Herald added a project: clang. The structural equivalence check stores now pairs of nodes in the 'from' and 'to' context instead of only the node in 'from'