Page MenuHomePhabricator

[clang-tidy] Cover cases like (b && c && b) in the redundant expression check
ClosedPublic

Authored by alexeyr on Fri, Jan 31, 4:44 AM.

Details

Summary

readability-redundant-expression could detect expressions where a logical or bitwise operator had equivalent LHS and RHS, but not ones where equivalent operands were separated by more operands.

Diff Detail

Event Timeline

alexeyr created this revision.Fri, Jan 31, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jan 31, 4:44 AM
alexeyr updated this revision to Diff 241696.Fri, Jan 31, 4:52 AM

Fixed test broken by git clang-format

alexeyr added a comment.EditedFri, Jan 31, 4:52 AM

Note: during testing I found that ++X && ++X is a false positive, my current implementation also introduces X && ++X && X (fixed in the last update).

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
335

Please don;e use else after return.

345

Could it be const auto *?

347

Please don't use auto when type is not spelled explicitly or iterator.

352

Please elide braces.

363

Please don't use auto when type is not spelled explicitly or iterator.

366

Please don't use auto when type is not spelled explicitly or iterator.

375

Please elide braces.

402

Please elide braces.

1246

Please elide braces.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
alexeyr added inline comments.Sun, Feb 2, 11:35 PM
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
363

In this case the type will depend on TExpr: either BinaryOperator::Opcode or OverloadedOperatorKind. I could make it a template parameter (but it won't be deducible) or convert OverloadedOperatorKind to Opcode. Any preference?

375

Should braces be elided from for as well?

alexeyr updated this revision to Diff 242000.Mon, Feb 3, 2:27 AM

Followed Eugene Zelenko's comments and fixed a false positive.

alexeyr marked 11 inline comments as done.Mon, Feb 3, 2:31 AM
alexeyr added inline comments.
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
363

Converted both to OverloadedOperatorKind (because I have a use for OO_None).

alexeyr marked an inline comment as done.Mon, Feb 3, 2:33 AM
aaron.ballman added inline comments.Mon, Feb 3, 5:07 AM
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
116

What do you think about the following?

bool foo(int&);
bool bar();

int i;
if (foo(i) && bar() && foo(i)) return 1;

I think that this should not be warned on (under the assumption that the reference variable can be modified by the call and thus may or may not be duplicate), but didn't see a test covering it.

It also brings up an interesting question about what to do if those were non-const pointers rather than references, because the data being pointed to could be modified as well.

(If you think this should be done separately from this review, that's totally fine by me, it looks like it would be an issue with the original code as well.)

One thing that is missing from this review are tests for the overloaded operator functionality.

alexeyr marked an inline comment as done.Mon, Feb 3, 7:12 AM
alexeyr added inline comments.
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
116

This is actually handled correctly, by the same logic as (X && X++ && X), so I don't think it needs a separate test. The drawback is that:

  1. it's too conservative, X && bar() && X isn't warned on either, because I don't know a way to check that bar() doesn't have side effects on X and so just test HasSideEffects (https://stackoverflow.com/questions/60035219/check-which-variables-can-be-side-effected-by-expression-evaluation-in-clang-ast).
  1. the original code does have the same issue and I didn't fix it, so foo(X) && foo(X) and X++ && X++ do get a warning.

I'll add overloaded operator tests.

aaron.ballman added inline comments.Mon, Feb 3, 10:07 AM
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
116

Okay, that seems reasonable to me, thank you!

alexeyr updated this revision to Diff 242266.Mon, Feb 3, 11:47 PM

Added tests for overloaded operators and fixed parent check logic.

alexeyr marked an inline comment as done.Mon, Feb 3, 11:52 PM
alexeyr added inline comments.
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
116

I've added the tests (which uncovered a problem not limited to overloaded operators; I needed to skip uninteresting nodes when looking at parents as well).

alexeyr marked an inline comment as done.Mon, Feb 3, 11:54 PM
alexeyr added inline comments.
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
116

Do you have any insight on https://reviews.llvm.org/D73775#1851553?

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

The only time I've seen that happen is when the range is invalid. I'm guessing you'll have to step through the diagnostics code to see what's going on.

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
1241

Diagnostics in clang tidy are not capitalized, so this should be overloaded operator and operator.

alexeyr added a comment.EditedWed, Feb 5, 5:21 AM

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

The only time I've seen that happen is when the range is invalid. I'm guessing you'll have to step through the diagnostics code to see what's going on.

After stepping through diagnostics code, I... don't understand how it is supposed to work. ClangTidyDiagnosticConsumer is

A diagnostic consumer that turns each Diagnostic into a SourceManager-independent ClangTidyError.

And it adds the hints to the ClangTidyError here:

for (const auto &FixIt : Hints) { ... }

But not the ranges, and I don't see any place where it does or even how they can be inserted into a ClangTidyError in the first place!

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

The only time I've seen that happen is when the range is invalid. I'm guessing you'll have to step through the diagnostics code to see what's going on.

After stepping through diagnostics code, I... don't understand how it is supposed to work. ClangTidyDiagnosticConsumer is

A diagnostic consumer that turns each Diagnostic into a SourceManager-independent ClangTidyError.

And it adds the hints to the ClangTidyError here:

for (const auto &FixIt : Hints) { ... }

But not the ranges, and I don't see any place where it does or even how they can be inserted into a ClangTidyError in the first place!

Huh, this does seem like it may be a bug in clang-tidy. I would have expected ClangTidyDiagnosticConsumer::forwardDiagnostic() to do this work.

alexeyr updated this revision to Diff 242572.Wed, Feb 5, 5:33 AM

Lower-case warning messages

Huh, this does seem like it may be a bug in clang-tidy. I would have expected ClangTidyDiagnosticConsumer::forwardDiagnostic() to do this work.

From my debugging attempt it seems not to be called (it's only supposed to be "If there is an external diagnostics engine, like in the ClangTidyPluginAction case"). Reported at https://bugs.llvm.org/show_bug.cgi?id=44799.

denis13 added a subscriber: denis13.Fri, Feb 7, 2:26 AM

Ping. Are any other changes needed?

aaron.ballman accepted this revision.Wed, Feb 12, 11:31 AM

LGTM aside from a minor nit.

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
355

No else after a return.

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
206

I think that this is reasonable behavior, however, once the user overloads the operator there's no way to know whether calling that operator has other side effects that make this not actually be equivalent to U && V. That said, unless this happens an awful lot in practice, this still seems like a reasonable heuristic.

This revision is now accepted and ready to land.Wed, Feb 12, 11:31 AM
alexeyr updated this revision to Diff 244370.Thu, Feb 13, 2:33 AM

Actually updated.

aaron.ballman accepted this revision.Thu, Feb 13, 9:23 AM

Still LGTM, thank you! If you need me to commit this on your behalf, I'm happy to do so, just let me know. It'll have to wait until next week, though (unless someone else does it first).

Yes, I do. Thank you (and @Eugene.Zelenko) for your help making this better!

alexeyr added a comment.EditedFri, Feb 14, 2:47 AM

@aaron.ballman I just noticed this in https://mlir.llvm.org/getting_started/Contributing/

Note: if you haven’t used Arcanist to send your patch for review, committers don’t have access to your preferred identity for commit messages. Make sure to communicate it to them through available channels or use the git sign-off functionality to make your identity visible in the commit message.

Can you please make sure the email is listed as romanov.alexey1@huawei.com when committing (I've just set it to primary, so it may be automatic?)

aaron.ballman closed this revision.Tue, Feb 18, 8:45 AM

@aaron.ballman I just noticed this in https://mlir.llvm.org/getting_started/Contributing/

Note: if you haven’t used Arcanist to send your patch for review, committers don’t have access to your preferred identity for commit messages. Make sure to communicate it to them through available channels or use the git sign-off functionality to make your identity visible in the commit message.

Can you please make sure the email is listed as romanov.alexey1@huawei.com when committing (I've just set it to primary, so it may be automatic?)

Absolutely -- thank you for clarifying what email address you wanted used. I've commit on your behalf in 5e7d0ebf735a8b70f92acd1f91c7c45423e611cc