Page MenuHomePhabricator

[clang-tidy][misc-redundant-expression] Support for CXXFoldExpr
ClosedPublic

Authored by zinovy.nis on May 31 2020, 12:10 PM.

Details

Summary

The revision adds a support for CXXFoldExpr allowing handling cases like

template <class... Values>
struct Bar2 {
  static_assert((... && (sizeof(Values) > 0)) || (... && (sizeof(Values) > 0)));
  //  warning: both sides of operator are equivalent [misc-redundant-expression]
};

Also fixes the bug https://bugs.llvm.org/show_bug.cgi?id=44256

Diff Detail

Event Timeline

zinovy.nis created this revision.May 31 2020, 12:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2020, 12:10 PM

Fix formatting.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a project: Restricted Project.

Fix and simplify the condition.

This fix has expanded from preventing the crash to adding support for CXXFoldExpr to misc-redundant-expression. Maybe rename the revision to explain that better.
If that is the case then you may as well add a test case showing a redundant fold expression

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
74–75

What happens when *LeftIter is null, but *RightIter isn't or vice versa, That case should also return false.

zinovy.nis updated this revision to Diff 267565.Jun 1 2020, 2:46 AM
  • Fix test.
  • Simplify the code.
zinovy.nis retitled this revision from [clang-tidy][misc-redundant-expression] Fix crash on CXXFoldExpr to [clang-tidy][misc-redundant-expression] Support for CXXFoldExpr.Jun 1 2020, 2:49 AM
zinovy.nis edited the summary of this revision. (Show Details)
zinovy.nis marked 2 inline comments as done.
zinovy.nis added inline comments.
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
18

I had to move this up as no warnings were generated in the original test code despite -fno-delayed-template-parsing.

This comment was removed by zinovy.nis.

@njames93, @aaron.ballman do you have any other comments?

njames93 added inline comments.Jun 4 2020, 1:53 AM
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
31

See other comment but if you change this to use == instead of || you can move this all back to the bottom simplifying the changes in this PR.

172

This is why you aren't getting warnings issued when the test case is at the bottom. You have defined a || operator at global namespace level which I'm guessing leads to ambiguity when checking the static_assert operator in your test case - It'll have a dependent type.
Simple fix is to change the static_assert to use say operator== as that isn't defined globally.

Simplify test.

zinovy.nis marked 2 inline comments as done.Jun 4 2020, 10:45 AM
zinovy.nis marked an inline comment as done.Jun 4 2020, 10:48 AM
zinovy.nis added inline comments.
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
172

It helped, thanks!

aaron.ballman accepted this revision.Jun 5 2020, 6:28 AM

LGTM, but please wait a bit in case @njames93 has any final thoughts.

This revision is now accepted and ready to land.Jun 5 2020, 6:28 AM
njames93 accepted this revision.Jun 5 2020, 8:12 AM

LGTM, nothing else to add.

This revision was automatically updated to reflect the committed changes.

How are you landing these changes, because the commit message isn't lining up with the PR name?

How are you landing these changes, because the commit message isn't lining up with the PR name?

It was my fault: I hadn't changed a title in my local branch and pushed it with git.