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