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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
