[PATCH] D145138: [clang-tidy] Implement FixIts for C arrays

[Piotr Zegar via Phabricator via cfe-commits]

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