[PATCH] D82089: [clang-tidy] modernize-loop-convert reverse iteration support

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Thanks for the patch! Looks generally good. A few comments inline.



================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:74-76
+static const char MakeReverseRangeFunction[] = "MakeReverseRangeFunction";
+static const char MakeReverseRangeHeader[] = "MakeReverseRangeHeader";
+static const char UseCxx20ReverseRanges[] = "UseCxx20ReverseRanges";
----------------
These are used in two places close together. I'd just use literals like for the other option names.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:921
+        &Descriptor.ContainerNeedsDereference,
+        /*IsReverse*/ FixerKind == LFK_ReverseIterator);
   } else if (FixerKind == LFK_PseudoArray) {
----------------
`/*IsReverse=*/`


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:1003
+      IsEnabled = true;
+      Function = RangesMakeReverse.str();
+      Header = RangesMakeReverseHeader.str();
----------------
I'd just use string literals directly.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:1011-1015
+  if (Header.front() == '<' && Header.back() == '>') {
+    IsSystemInclude = true;
+    Header.pop_back();
+    Header.erase(0, 1);
+  }
----------------
It looks like this should be a feature of the `IncludeInserter`. Not necessarily in this patch though. The `createIncludeInsertion` in other checks could be made a bit more self-descriptive too, e.g. this one in clang-tidy/modernize/MakeSmartPtrCheck.cpp:
```
  Diag << Inserter.createIncludeInsertion(
      FD, MakeSmartPtrFunctionHeader,
      /*IsAngled=*/MakeSmartPtrFunctionHeader == StdMemoryHeader);
```
could have angle brackets in `MakeSmartPtrFunctionHeader` instead of making a special case for `memory`.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h:42-63
+
+  class ReverseRangeDetail {
+  public:
+    ReverseRangeDetail(bool UseCxx20IfAvailable, std::string FunctionName,
+                       std::string HeaderName, const LangOptions &LangOpts);
+
+    bool isEnabled() const { return IsEnabled; }
----------------
This abstraction doesn't seem to give much benefit over placing all these fields into the `LoopConvertCheck` class. 


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h:58
+  private:
+    bool IsEnabled{false};
+    bool UseCxx20IfAvailable;
----------------
Use ` = false;` for initialization. It's more common in LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82089