[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

> There are already two way more sophisticated (forgive me my bias) implementations in Clang that are for checking if two statements or decls are the same.
>
> 1. ODRHash, used in modules to discover ODR violations
> 2. ASTStructuralEquivalenceContext, used in ASTImporter to discover if two AST nodes are the same or not (as a side effect we diagnose ODR violations as well).
>
> It is not the first time, when such a similarity check is needed (see https://reviews.llvm.org/D75041). Of course reusing the before mentioned components would require some architectural changes, but it might be beneficial.

I do not quite see the overlap.  This patch addresses the structural equivalence of DeclRefExprs: as the `std::is_same` (or any type trait example) demonstrates, two declarations may be the "same" (e.g. they are both `std::false_type::value`), but two DeclRefExprs referring to those declarations should not necessarily be considered the "same": the qualifier, specifying the path that was taken to look them up, can matter to a user.  It's not a matter of the sophistication of the similarity check, it's a matter of what we mean by similarity.

I do not see DeclRefExprs handled in ODRHash or ASTStructuralEquivalence.  I do see NestedNameSpecifiers handled in both, but I don't think the implementation quite matches what is needed here (e.g. in ASTStructuralEquivalence check, if one NNS is a NamespaceAlias, the other one is assumed to be a NamespaceAlias: not what we want).  It's probably not worth the trouble to factor something common out of those; though they should certainly be used as a guide to make sure no cases have been missed.


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

https://reviews.llvm.org/D114622