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

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

balazske added a comment.

In D66538#1641310 <https://reviews.llvm.org/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 decl chain, then (A,B) or (A,C) will be non-equivalent (unless B and C are equivalent, but what to do in this case?). The problem was that the code assumed that in this case always A and B (or A and C) are non-equivalent. If NonEquivalentDecls is not filled in this case (or not used at all) the problem does not exist.
>
> I think we must not update the NonEquivalentDecls cache at that level, because as we've seen (during the discussion of https://reviews.llvm.org/D62131) it may store the wrong pairs.
>  However, it is still okay to cache the inequivalent decls in one level upper, right after the `Finish` returns.
>  See this diff https://github.com/martong/clang/compare/NonEqDecls_cache_in_an_upper_level~...martong:NonEqDecls_cache_in_an_upper_level?expand=1
>  Right now I started a build on our CI to see if it causes any slowdown.


This is correct, `NonEquivalentDecls` can be filled after `Finish` (in `IsEquivalent`?) (still we can have similar cache for equivalence too, maybe this is not of so much use). With the new code there is the possibility to get more information about non-equivalence, the `NonEquivalentDecls` can be filled in `Finish` too like it is done now, and with not much effort (I think) we can add some dependency relation (a tree structure) to decide which (already visited) Decls become non-equivalent if one non-equivalence is found. If there is a `void f(A *, B *)` to check the `f` can be a "parent" of `A` and `B`, if `A` or `B` is found to be non-equivalent we can set this for `f` too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66538