[PATCH] D116577: [clang-tidy] Added "boost-use-range-based-for-loop" check

[Richard via Phabricator via cfe-commits]

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