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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
