LegalizeAdulthood added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/boost/CMakeLists.txt:8
BoostTidyModule.cpp
+ UseRangeBasedForLoopCheck.cpp
UseToStringCheck.cpp
----------------
I am wondering if this check is better placed in the modernize module?
Maybe as an enhancement to the existing `modernize-loop-convert`?
================
Comment at: clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp:17
+#include <numeric>
+
+namespace clang {
----------------
`#include <memory>` for `std::make_unique<>`
================
Comment at: clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp:39
+ const MacroArgs *Args) override {
+ IdentifierInfo *NameIdentifierInfo = MacroNameToken.getIdentifierInfo();
+ if (!NameIdentifierInfo)
----------------
Can this be `const IdentifierInfo *`?
================
Comment at: clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp:64
+
+ Diag << FixItHint::CreateReplacement(
+ CharSourceRange::getCharRange(MacroNameToken.getLocation(),
----------------
Can't you just chain this with the previous statement?
So....
```
Check->diag(...)
<< MacroName
<< FixItHint::CreateReplacement(...);
```
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst:30
+-----------------
+* Client code that use ``BOOST_FOREACH`` with "``std::pair`` of iterators" or
with "Null-terminated strings (``char`` and ``wchar_t``)" or some other
sequence types (everything that is not declared in the list "convertible
correctly" above) will produce broken code (compilation error).
+
----------------
Eugene.Zelenko wrote:
> Please separate with newline and follow 80 character limit. Also closing
> quote for `Null-terminated strings` is missing.
It would be better to detect known failure cases in the matcher and not issue a
fixit but perhaps issue a diagnostic.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst:47
+
+* First argument of the ``BOOST_FOREACH`` macro must be only new identifier
definition, all other will produce a compilation error after migration.
+
----------------
Can't you detect this in the matcher and not issue a fixit in that case?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/boost-use-range-based-for-loop.cpp:9
+#define BOOST_FOREACH(VAR, COL) while(true)
+#define BOOST_REVERSE_FOREACH(VAR, COL) while(true)
+
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116577/new/
https://reviews.llvm.org/D116577
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
