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

[Denis Mikhailov via Phabricator via cfe-commits]

denzor200 marked an inline comment as done.
denzor200 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/boost/CMakeLists.txt:8
   BoostTidyModule.cpp
+  UseRangeBasedForLoopCheck.cpp
   UseToStringCheck.cpp
----------------
LegalizeAdulthood wrote:
> I am wondering if this check is better placed in the modernize module?
> Maybe as an enhancement to the existing `modernize-loop-convert`?
I vote against the "modernize" section for this check. See arguments above. 


================
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).
+
----------------
LegalizeAdulthood wrote:
> 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.
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.


================
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.
+
----------------
LegalizeAdulthood wrote:
> Can't you detect this in the matcher and not issue a fixit in that case?
Yes, i can just parse the string token `char c` into mini AST and then determine if this is a variable declaration.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116577/new/

https://reviews.llvm.org/D116577