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

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

steakhal added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-55
                                        const NestedNameSpecifier *Right) {
-  llvm::FoldingSetNodeID LeftID, RightID;
-  Left->Profile(LeftID);
-  Right->Profile(RightID);
-  return LeftID == RightID;
+  if (const auto *L = Left->getAsType())
+    if (const auto *R = Right->getAsType())
+      return L->getCanonicalTypeUnqualified() ==
----------------
whisperity wrote:
> whisperity wrote:
> > (Wrt. to the comment about not typing stuff out directly, I guess `L` and `R` here will be `X<char>` and `X<long>` and thus not the same.)
> (Style nit to lessen the indent depth, until C++17...)
> (Wrt. to the comment about not typing stuff out directly, I guess L and R here will be X<char> and X<long> and thus not the same.)
I don't quite get what do you mean by this.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:132-134
+    const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Left);
+    const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Right);
+    return areEquivalentDeclRefs(DeclRef1, DeclRef2);
----------------
whisperity wrote:
> (Style nit. Or `LeftDR`, `RightDR`.)
`DeclRef1` -> `LeftDR`


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:822
+
+  using my_char = char;
+  sink |= my_trait<char>::value || my_trait<my_char>::value;
----------------
whisperity wrote:
> Are there any more ways we could trick this check to cause false positives? Something through which it doesn't look the same but still refers to the same thing?
> 
> At the top of my head I'm thinking about something along the lines of this:
> 
> ```lang=cpp
> template <typename T>
> struct X {
>   template <typename U>
>   static constexpr bool same = std::is_same_v<T, U>;
> };
> 
> X<char> ch() { return X<char>{}; }
> X<int> i() { return X<int>{}; }
> 
> void test() {
>   coin ? ch().same<long> : i().same<long>;
> }
> ```
> 
> So the "type name" where we want to look up isn't typed out directly.
> Or would these still be caught (and thus fixed) by looking at a `NestedNameSpecifier`?
For the example:
```lang=C++
template <typename T, T V> struct integral_constant {
    static constexpr T value = V;
    typedef T value_type;
    typedef integral_constant<T, V> type;
};
typedef integral_constant<bool, true> true_type;
typedef integral_constant<bool, false> false_type;
template <typename T, typename U> struct is_same : false_type {};
template <typename T> struct is_same<T, T> : true_type {};

template <typename T>
struct X {
  template <typename U>
  static constexpr bool same = is_same<T, U>::value;
};

X<char> ch();
X<int> i();

void test(bool coin) {
  coin ? ch().same<long> : i().same<long>;
}
```
We would have no warnings for this code, nor we had previously:
https://godbolt.org/z/9xo4adfbP

Should I demonstrate this in the test file?
I wouldn't pollute it TBH, since it requires some template machinery which is quite verbose.
However, if you insist I will add it.


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

https://reviews.llvm.org/D114622