Page MenuHomePhabricator

[clang-tidy] add new check readability-use-anyofallof
AcceptedPublic

Authored by mgehre on Apr 6 2020, 10:38 AM.

Details

Summary

Finds range-based for loops that can be replaced by a call to std::any_of` or
std::all_of. In C++ 20 mode, suggests std::ranges::any_of or
std::ranges::all_of.
For now, no fixits are produced.

Diff Detail

Event Timeline

mgehre created this revision.Apr 6 2020, 10:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 10:38 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
27

Please don't use auto unless type is spelled in same statement or iterator.

56

Must be in isLanguageVersionSupported() method.

111

Unnecessary empty line.

clang-tools-extra/docs/ReleaseNotes.rst
135

Please enclose std::any_of and std::all_of in double back-ticks.

Eugene.Zelenko added a project: Restricted Project.
njames93 added a comment.EditedApr 6 2020, 1:30 PM

As no FixIts are made the IncludeInserter can be removed.

I'm struggling to see the value of this check though. If it was reworked to check for loop in the middle of a function it would have a lot more value.

bool all_of = true;
for (auto X : V) {
  if (!X) {
    all_of = false;
    break;
  }
}

Being able to identify those and possibly even transform them would have much more value

bool all_of = std::all_of(std::begin(V), std::end(V), [](const auto &X) { return static_cast<bool>(X) });

I'm guessing you'd want to check for compound statements that have a bool VarDecl with an init just before the range for loop and a condition in the loop that flips the value and then breaks after.

clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
22–42

This should be in an anonymous namespace as it doesn't need external linkage. Probably shouldn't be in the ast_matchers namespace either

35

nit: const auto *I = llvm::find(C->body(), &Node);

See also suggestion for more generic loop-to-algorithm transformations.

mgehre edited the summary of this revision. (Show Details)Apr 6 2020, 11:36 PM
mgehre marked 6 inline comments as done.Apr 7 2020, 2:14 PM

I'm struggling to see the value of this check though. If it was reworked to check for loop in the middle of a function it would have a lot more value.

This is (just) a first step. The next step is to detect the local variable case as you also described it. Note that this also catches functions
that do some preprocessing and end with a any_of-style loop.
I also have a local branch that generates fixits, but they add quite some code, so I wanted to put them in a separate PR.

In LLVM & clang, the check in this PR already emits 370 unique warnings.

mgehre updated this revision to Diff 255812.Apr 7 2020, 2:18 PM

Review comments

mgehre updated this revision to Diff 255814.Apr 7 2020, 2:19 PM

Review comments

Harbormaster completed remote builds in B52234: Diff 255814.

I'm struggling to see the value of this check though. If it was reworked to check for loop in the middle of a function it would have a lot more value.

This is (just) a first step. The next step is to detect the local variable case as you also described it. Note that this also catches functions
that do some preprocessing and end with a any_of-style loop.
I also have a local branch that generates fixits, but they add quite some code, so I wanted to put them in a separate PR.

In LLVM & clang, the check in this PR already emits 370 unique warnings.

Take this example from TableGen/Record.cpp:

bool CondOpInit::isConcrete() const {
  for (const Init *Case : getConds())
    if (!Case->isConcrete())
      return false;

  for (const Init *Val : getVals())
    if (!Val->isConcrete())
      return false;

  return true;
}

Firstly, the warning is only emitted on the second loop.
Secondly how does your fix it code handle temporaries?
Would it transform to

bool CondOpInit::isConcrete() const {
  for (const Init *Case : getConds())
    if (!Case->isConcrete())
      return false;

  return std::all_of(getVals().begin(), getVals().end(),
                     [](const Init *Val) { return Val->isConcrete(); });
}

I'd argue that it actually makes the code less readable as there are 2 different constructs for the same thing.

mgehre added a comment.EditedApr 10 2020, 2:31 PM

Thanks @njames93, this is a good examples which I did not consider yet. I agree that only transforming
the second loop would make the code worse.

I would argue that a human seeing a warning on the second loop might also realize
that the first loop can be transformed in a similar way, and possibly combine them into

return llvm::all_of(getConds(), [](const Init *Cond) { return Val->isConcrete(); }) && llvm::all_of(getVals(), [](const Init *Val) { return Val->isConcrete(); });

or

auto IsConcrete = [](auto Init* I) { return I->isConcrete(); };
return llvm::all_of(getConds(), IsConcrete) && llvm::all_of(getVals(), IsConcrete);

(I was wondering whether the check should have an option to suggest llvm::all_of (or boost::algorithm::all_of, ...) instead of std::all_of, but
I thought that this could go into another PR.)

I have the feeling that extracting code into algorithms is generally a hard topic,
and automatic fixits would possible give a false sense of automatism (at least at the current point).
Your example is a good reminder of that.

Personally, clang-tidy has been a good source of learning C++ best practices. And I hope that this clang-tidy check would help
me and my coworkers to remember using those algorithms.

Are you saying that this check should not land unless its scope is extended? What would be the minimal scope making this check
worth-while to land?

Right now it would be a great candidate for this - http://lists.llvm.org/pipermail/cfe-dev/2020-March/064980.html, however in its current state I wouldn't say its ready to get the green light right now.
No point worrying about the fix-its yet, but when it is time, could add options for RangeAllOfName, RangeAnyOfName, RangeNoneOfName, AlgorithmHeader.
Gonna throw it out there that generating that templated lambda automatically... just don't :)

Thanks for the comments so far.
I'm a bit lost now. Which changes that cannot wait for the next PR do you see necessary to get this check merged?

Ping :-)
I'm looking for directions on what the next steps are.

Thanks for the comments so far.
I'm a bit lost now. Which changes that cannot wait for the next PR do you see necessary to get this check merged?

I'd be curious to know what @njames93 thinks -- I spot-checked the diagnostics you attached earlier (thank you for those!) and all of them seemed reasonable to me, which suggests there's not an extraordinary amount of false positives. The functionality seems useful in its current state, though as you point out, there are improvements you want to make in follow-up patches. That seems reasonable to me.

clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
23–25

It looks like these comments got formatted a bit strangely -- probably should re-flow them.

37

I think the comment should turn into a string literal that's part of the assertion.

62

Should we reject other ways to break out of the loop, like goto or throw?

99

clang-tidy diagnostics don't start with a capital letter, and I'd probably drop the from <algorithm> part under the assumption the user can figure out the header pretty easily from the diagnostic wording. Also, you should put single quotes around the std%0::any_of(). Similar below.

mgehre marked 5 inline comments as done.Mon, May 25, 3:17 PM

Aaron, thank you very much for taking the time to review this PR.

clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
62

I think throw statements still can be transformed. We cannot transform break because the loop is gone and we cannot transform goto because we cannot jump from the lambda into its caller.
But we can keep throw statements because exceptions can propagate from the lambda through the algorithm back into the original caller. If we could not allow throw statements, we would also have to disallow any other kind of call statements.

mgehre updated this revision to Diff 266083.Mon, May 25, 3:17 PM
mgehre marked an inline comment as done.

Fix comments

aaron.ballman accepted this revision.Sat, May 30, 9:45 AM

LGTM, though please wait a bit for @njames93 to speak up if they still have concerns.

clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
62

Ah, good point on throw, thank you!

This revision is now accepted and ready to land.Sat, May 30, 9:45 AM
njames93 accepted this revision.Sat, May 30, 10:52 AM

LGMT, just a few minor nits though.

clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
15–16

Fairly certain these headers aren't used

clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
30

std::all_of, std::any_of and std::none_of were only introduced in c++11, maybe CPlusPlus11 should be the minimum requirement.

clang-tools-extra/docs/ReleaseNotes.rst
202

This removed line in unrelated and should be added back