[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-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59
+           RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.
----------------
steakhal wrote:
> davrec wrote:
> > It occurs to me this needs to be recursive, to check a sequence of qualifiers, i.e.
> > ```
> >   const Type *LTy = Left->getAsType();
> >   const Type *RTy = Right->getAsType();
> >   if (!LTy || !RTy) 
> >     return true;
> >   if (LTy->getCanonicalTypeUnqualified() !=
> >        RTy->getCanonicalTypeUnqualified())
> >     return false;
> >   return areEquivalentNameSpecifier(Left->getPrefix(), Right->getPrefix());
> > ```
> > 
> > The reason is, if there is a prefix qualifier to this qualifier, we run into the original problem, e.g.:
> > ```
> > struct Base {
> >   struct Inner { 
> >     static const bool value = true;
> >   };
> > };
> > struct Outer1 : Base {};
> > struct Outer2 : Base {};
> > 
> > // We do not want the following to warn, but without checking the prefix of Inner, 
> > // I believe these will be interpreted as equivalent DeclRefs and will warn:
> > auto sink = Outer1::Inner::value || Outer2::Inner::value;
> > ```
> > 
> You are right, but that would require me to implement all the possible kinds of NNS, in addition to the types.
> And it's not entirely clear to me (yet) what the desired behavior should be for implementing this.
I agree it's not entirely clear what should be the desired behavior in that rare case; I suppose it might be appropriate to introduce a `bool RecurseToPrefix` param that determines whether you do the while loop or finish after one iteration, mostly so that others using this function can ponder the decision for themselves.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372
+}
+
 /// Determines whether two statement trees are identical regarding
----------------
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) { ... }
};

```


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

https://reviews.llvm.org/D114622