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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Sorry for my late response, I'm busy with other things right now. I plan to come back to this in a couple of weeks.



================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:52
 
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
                                        const NestedNameSpecifier *Right) {
----------------
davrec wrote:
> 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.)
> 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.
I agree. I just wanted to preserve as much code as I could and aim for a minimal change to implement my intention.
I'll update this accordingly.

> (*Is this indeed the desired behavior? Probably should add some tests that check qualified DeclRefExprs against unqualified DeclRefExprs, to be sure.)
You are right, not perfect. But it's already closer than it previously was. I'll add the test anyway.


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


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

https://reviews.llvm.org/D114622