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

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372
+}
+
 /// Determines whether two statement trees are identical regarding
----------------
davrec wrote:
> I would strongly support turning these functions into methods of DeclRefExpr and NestedNameSpecifier, not just to avoid the code duplication here but because they sure seem to be of general utility to AST users.
> 
> You just need to clearly motivate these methods in the documentation, which your description for this patch largely does, e.g. something like.:
> 
> ```
> class DeclRefExpr : ... {
>   ...
>   /// Returns whether this \c DeclRefExpr 
>   ///   a. refers to the same canonical declaration as \c other and
>   ///   b. if it has a qualifier, that qualifier refers to the same canonical 
>   ///       declarations as those of \c other .
>   ///
>   /// Consider these two DeclRefExprs:
>   /// \code
>   ///   std::is_same<char, int>::value
>   ///   std::is_same<char, long>::value;
>   /// \endcode
>   ///
>   /// We can see that the value static members are different, but in fact
>   /// the \c getDecl() of these two returns the same VarDecl. (...etc)
>   bool isSemanticallyEquivalentTo(const DeclRefExpr *Other) { ... }
> };
> ...
> class NestedNameSpecifier {
>   ...
>   /// Returns whether this refers to the same sequence of identifiers/canonical declarations
>   /// as \c Other.  (Then I would repeat the std::is_same example here, since that
>   /// really makes the issue clear.  And also describe what this returns when other is nullptr or
>   /// when getPrefix() is nullptr while other->getPrefix() is non-null -- maybe add a parameter if
>   /// its not clear in general what the behavior should be.)
>   bool isSemanticallyEquivalentTo(const NestedNameSpecifier *Other, bool RecurseToPrefix = true) { ... }
> };
> 
> ```
Also if doing this it might be nice to add `isSyntacticallyEquivalentTo(other)` methods alongside the semantic versions, defined the same except they don't desugar the type nor get the underlying NamespaceDecl from a NamespaceAliasDecl.  This would be simple and, given that you found that two NestedNameSpecifiers representing the exact same syntax are nonetheless different pointers, it could be very useful for AST users concerned with syntax rather than semantics.

(Btw note the DeclRefExpr's syntactic method should still use getDecl()->getCanonicalDecl(), since, confusingly, getCanonicalDecl simply fetches the FirstDecl from the various redeclarations, rather than doing any kind of desugaring as getCanonicalType does.  And fortunately I don't think it is necessary to manually "desugar" the canonical decl in the semantic version, since a DeclRefExpr's getDecl is always a ValueDecl, which I don't think can ever be "sugar" for some underlying decl, as say a NamespaceAliasDecl is for a NamespaceDecl -- so that part should be fine.)



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

https://reviews.llvm.org/D114622