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.
Details
- Reviewers
aaron.ballman njames93 LegalizeAdulthood
Diff Detail
Event Timeline
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? | |
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?
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.
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. |
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, However, my bigger question is what happens when someone I looked at the header for LangOptions and it isn't clear whether | |
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 I suppose you could match for loops and try to correlate them to |
clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.h | ||
---|---|---|
29 | When you specify a standard on the command-line, the front end |
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 |
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 |
Clang-Tidy tests and docs have moved to module subdirectories.
Please rebase this onto main:HEAD and:
- fold your changes into the appropriate subdirs, stripping the module prefix from new files.
- make the target check-clang-extra to validate your tests
- make the target docs-clang-tools-html to validate any docs changes
I am wondering if this check is better placed in the modernize module?
Maybe as an enhancement to the existing modernize-loop-convert?