[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.

A couple thoughts/cases to consider...



================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:52
 
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
                                        const NestedNameSpecifier *Right) {
----------------
This function should probably either be made into a lambda private to `areEquivalentDeclRefs()`, or renamed to make clear it does not apply generically to all NNS's.  

Part of the reason is that this is only implemented for type NNS's.  But the more important reason is that, when either a) `!Left || !Right`, or b) `!Left->getAsType() || !Right->getAsType()`, this returns true, since (presumably*) this gives us the desired behavior within areEquivalentDeclRefs(), despite that in general a null NNS should probably not be considered the same as a nonnull NNS.  

(*Is this indeed the desired behavior? Probably should add some tests that check qualified DeclRefExprs against unqualified DeclRefExprs, to be sure.)


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:53
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
                                        const NestedNameSpecifier *Right) {
+  const Type *LTy = Left->getAsType();
----------------
Suggest you move the null check from `areEquivalentDeclRefs()` here, i.e.
```
if (!Left || !Right)
  return true;
```
mainly since this is needs to be done in recursive calls (see below), but also since you do the same logic on LTy and RTy subsequently.




================
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.
----------------
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;
```



================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:72
+  // The decls were already matched, so just return true at any later point.
+  if (!Left->hasQualifier() || !Right->hasQualifier())
+    return true;
----------------
Suggest you move this null check into areEquivalentNameSpecifier, see above


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

https://reviews.llvm.org/D114622