Page MenuHomePhabricator

[clang-tidy] Added "boost-use-range-based-for-loop" check
Changes PlannedPublic

Authored by denzor200 on Jan 3 2022, 11:28 PM.

Details

Summary

Added a check for converting `BOOST_FOREACH(..., ...)` loops to use the new range-based loops in C++11.
This check have some limitations, see documentation for details.

Diff Detail

Event Timeline

denzor200 created this revision.Jan 3 2022, 11:28 PM
denzor200 requested review of this revision.Jan 3 2022, 11:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 11:28 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp
61

Please specify type explicitly here.

clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.h
23

Please add isLanguageVersionSupported.

clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst
7

Please synchronize this statement with statement in Release Notes.

22

It will be good idea to move it to separate code block and add something like from and to. See modernize checks as example. Same for other examples.

31

Please separate with newline and follow 80 character limit. Also closing quote for Null-terminated strings is missing.

clang-tools-extra/clang-tidy/boost/CMakeLists.txt
8

I am wondering if this check is better placed in the modernize module?
Maybe as an enhancement to the existing modernize-loop-convert?

clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp
18

#include <memory> for std::make_unique<>

40

Can this be const IdentifierInfo *?

65

Can't you just chain this with the previous statement?

So....

Check->diag(...)
    << MacroName
    << FixItHint::CreateReplacement(...);
clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst
31

It would be better to detect known failure cases in the matcher and not issue a fixit but perhaps issue a diagnostic.

48

Can't you detect this in the matcher and not issue a fixit in that case?

clang-tools-extra/test/clang-tidy/checkers/boost-use-range-based-for-loop.cpp
10

Why define this (and foreach_r_) if you don't have any tests around it?

Do you intend to support a conversion for the reverse iteration?

I opened a similar issue for converting Qt's foreach to a range for loop.

However this check lands, it should be a simple generalization to have it process Qt foreach loops as well, so perhaps a check name like modernize-foreach-to-range-for would be better?

I opened a similar issue for converting Qt's foreach to a range for loop.

However this check lands, it should be a simple generalization to have it process Qt foreach loops as well, so perhaps a check name like modernize-foreach-to-range-for would be better?

Generic code is a good practize, but i vote against the modernize-foreach-to-range-for check. My arguments:
-Not everyone uses boost and not everyone agrees to see "boost checks" in the already large modernize section. In the future, there will be a lot of these checks, not only BOOST_FOREACH. This is for example Boost Assign, Boost Static Assert, Boost Format, and so on, you can go on and on.
-All the same as above can be said about Qt.

denzor200 updated this revision to Diff 398516.Jan 9 2022, 11:17 PM
denzor200 marked 9 inline comments as done.

review

denzor200 updated this revision to Diff 398517.Jan 9 2022, 11:20 PM

clang-format

denzor200 marked an inline comment as done.Jan 9 2022, 11:36 PM
denzor200 added inline comments.
clang-tools-extra/clang-tidy/boost/CMakeLists.txt
8

I vote against the "modernize" section for this check. See arguments above.

clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst
31

I do not understand how I can determine type of expression list_int (for example) at the preprocessing stage. This trick was not found in any of the existing checks.

48

Yes, i can just parse the string token char c into mini AST and then determine if this is a variable declaration.

denzor200 marked an inline comment as done.Jan 9 2022, 11:36 PM
clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp
69

Now that this is chained, you might need to clang-format on it again.

Probably best to just clang-format the whole file before you submit.

clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.h
29

This is an implicit conversion from unsigned to bool,
an explicit comparison against != 0 would be preferable IMO.

However, my bigger question is what happens when someone
runs this check with -std=c++14 or some other later version of C++?

I looked at the header for LangOptions and it isn't clear whether
the bits are set to indicate supported portions of the standard or
if they simply reflect the command-line options used when invoking
the compiler.

clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst
31

Ah, yes, ugh. I forgot that this is all done by looking at macros.

We really need a bit of infrastructure in clang-tidy to associate macro
expansions with AST nodes for preprocessor related checks.

I suppose you could match for loops and try to correlate them to
the BOOST_FOREACH macros. Then you could reason more about
the arguments to BOOST_FOREACH via the AST, but this is probably
fragile because it depends on the implementation of BOOST_FOREACH,
which may change.

clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.h
29

When you specify a standard on the command-line, the front end
sets all the relevant bits, so if you say -std=c++14 then C++11 is
also set (but not C++17, C++20, etc.)

denzor200 updated this revision to Diff 411548.Feb 25 2022, 4:06 PM
denzor200 marked 6 inline comments as done.

My apologize for the delay! Diff was updated, implemented detection of known failure cases in the matcher and not issue a fixit, just only diagnostic.

clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp
69

clang-format was already applied

denzor200 updated this revision to Diff 411556.Feb 25 2022, 5:03 PM
denzor200 planned changes to this revision.Feb 26 2022, 4:14 AM
denzor200 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/boost-use-range-based-for-loop.cpp
22

Unfortunately, example does not correspond to the real BOOST_FOREACH, the tests do not reveal real problems. Will have to redo. I will come back to this patch in the future

denzor200 marked an inline comment as not done.Feb 26 2022, 4:16 AM