PiotrZSL added a comment.
To be honest, that's lot of hard to understand code that at start is
unmaintainable.
I don't see anyone even trying to understand all this and do some fixes in
future.
Consider some refactoring, to reduce amount of code, split it into some
functions, add some comments (steps) inside functions.
I hope you will be with us for next 2 years to implement fixes...
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:54-57
+ anyOf(hasParent(varDecl(hasParent(declStmt().bind("decl")),
+ hasAncestor(functionDecl()))
+ .bind("var")),
+ anything()),
----------------
use optionally instead of anyOf + anything
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:250-254
+ if (ConsumedStar) {
+ return Token.IsSpecifier;
+ } else {
+ return Token.IsSpecifier || Token.IsQualifier;
+ }
----------------
style:
```
return Token.IsSpecifier || (!ConsumedStar && Token.IsQualifier);
```
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:400-405
+ if (Match.getNodeAs<CXXForRangeStmt>("range-for")) {
+ continue;
+ }
+ if (Match.getNodeAs<UnaryExprOrTypeTraitExpr>("sizeof")) {
+ continue;
+ }
----------------
style: remove {}
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:447-449
+ } else {
+ return false;
+ }
----------------
style: else return false;
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:466-470
+ if (ElemType->isArrayType())
+ return {};
+ if (ElemType->isFunctionPointerType() ||
+ ElemType->isMemberFunctionPointerType())
+ return {};
----------------
style: merge into single if
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:496
+ const Expr *SpelledExpr = E->IgnoreUnlessSpelledInSource();
+ if (dyn_cast<InitListExpr>(SpelledExpr))
+ return false;
----------------
InitListExpr::classof(SpelledExpr)
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:531
+ return {};
+ // TODO: How can I get FileCheck to accept '{{}}' as a non-regex match?
+ FixIts.push_back(FixItHint::CreateInsertion(InitRange.getBegin(), "{ "));
----------------
https://www.llvm.org/docs/CommandGuide/FileCheck.html
"In the rare case that you want to match double braces explicitly from the
input, you can use something ugly like {{[}][}]}} as your pattern."
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:540
+
+ std::string Replacement;
+ std::optional<FixItHint> IncludeFixIt =
----------------
unused
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:544
+ "<array>");
+ if (IncludeFixIt) {
+ FixIts.push_back(std::move(*IncludeFixIt));
----------------
remove {}
================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp:588
+ : ClangTidyCheck(Name, Context),
+ IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
+ utils::IncludeSorter::IS_LLVM),
----------------
ccotter wrote:
> Eugene.Zelenko wrote:
> > ccotter wrote:
> > > Eugene.Zelenko wrote:
> > > > Should be global option, because it's used in other checks.
> > > Could you clarify this a bit? This is how most other tests consume
> > > IncludeStyle (`Options.getLocalOrGlobal("IncludeStyle",
> > > utils::IncludeSorter::IS_LLVM)`.
> > @carlosgalvezp is best person to answer because he recently introduced
> > global option for source files and headers.
> bump @carlosgalvezp
```
Local:
- key: modernize-avoid-c-arrays.IncludeStyle
value: google
Global:
- key: IncludeStyle
value: google
```
Your code is correct, simply if there is local defined, it will be used, if
there is global defined, then global will be used.
================
Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp:94
- while (Loc < Range.getEnd()) {
+ while (SM.isBeforeInTranslationUnit(Loc, Range.getEnd())) {
if (Loc.isMacroID())
----------------
Actually this function is bugged: https://reviews.llvm.org/D146881
I dont think that isBeforeInTranslationUnit is needed here, if you need it then
there is something wrong with a range.
================
Comment at: clang-tools-extra/clang-tidy/utils/TypeUtils.cpp:50-51
+
+ bool Qual = isCvr(T);
+ bool Spec = isSpecifier(T);
+ CT.IsQualifier &= Qual;
----------------
style: const
================
Comment at: clang-tools-extra/clang-tidy/utils/TypeUtils.cpp:112-114
+ else {
+ return std::nullopt;
+ }
----------------
Style: no need for {}
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp:170-172
+ char init4[] = "abcdef";
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays,
use std::array<> instead
+ // CHECK-FIXES: std::array<char, 7> init4 = { "abcdef" };
----------------
I don't think this check should replace those strings, simply that could break
code, image someone is using this later as an argument to some function that
expect pointer.
For strings std::string_view would be better alternative (at least add a bypass
for this).
And all those changes are not safe if variable is used for pointer arithmetic.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145138/new/
https://reviews.llvm.org/D145138
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
