[clang-tidy] Implement FixIts for C arrays

[ccotter]

Summary

Add FixIts for C arrays declared in function definitions.
In this change, we'll generate FixIts for statically sized arrays
but not variable sized arrays. There are a few other edge cases not
implemented, but this change covers most common cases.

For demonstration/testing purposes, https://github.com/llvm/llvm-project/commit/9bd02233a35e358c4a69b7773dbaf98df65caa86 is the result of running the tool on llvm-project's llvm, clang, and clang-tools-extra directories. These changes compile cleanly after applying the fixits.

[ccotter]

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

531

I could not figure out how to get FileCheck to handle // CHECK-FIXES: std::array<int, 3> ar = {{0,1,2}}; without interpreting {{ as the beginning of a regex. Is there a way to escape one of the curly braces?

[ccotter]

• Add more tests for whitespace

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

93

Please do not use auto unless type is explicitly stated in same statement or iterator.

497

auto could be used here.

573

Should be global option, because it's used in other checks.

clang-tools-extra/docs/clang-tidy/checks/modernize/avoid-c-arrays.rst

71–79

Missing documentation for IncludeStyle option if it'll remain local.

[ccotter]

• Add option doc, remove auto

[ccotter]

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

573

Could you clarify this a bit? This is how most other tests consume IncludeStyle (Options.getLocalOrGlobal("IncludeStyle", utils::IncludeSorter::IS_LLVM).

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

573

@carlosgalvezp is best person to answer because he recently introduced global option for source files and headers.

[ccotter]

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

573

bump @carlosgalvezp

[PiotrZSL]

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...

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

54–57

use optionally instead of anyOf + anything

249–253

style:

return Token.IsSpecifier || (!ConsumedStar && Token.IsQualifier);

399–404

style: remove {}

446–448

style: else return false;

465–469

style: merge into single if

495

InitListExpr::classof(SpelledExpr)

530

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."

539

unused

543

remove {}

573

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.

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

94

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.

clang-tools-extra/clang-tidy/utils/TypeUtils.cpp

51–52

style: const

113–115

Style: no need for {}

clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp

170–172

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.

[ccotter]

• feedback

[ccotter]

rebase

[ccotter]

• Fix double curly brace matching in tests

[ccotter]

• Add tests for char[]

[ccotter]

clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp

170–172

The logic I implemented should fix pointer arithmetic (e.g., str + 1 is replaced by str.begin() + 1. I have tests for this for int[], but added more for char[]. string_view is not appropriate conversion here since the arrays may be modified etc.

[ccotter]

• format

[ccotter]

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

495

Why classof vs dyn_cast? The codebase seems to overwhelmingly use dyn_cst, and I need to use the downcast type anyway..

530

Thanks..fixed!

[carlosgalvezp]

clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp

573

Yep, this is similar to HeaderFileExtensions, current state is that all checks duplicate the same code when it should instead be just a global option. Since the global option is not available yet, let's leave it as it is and I will put up a patch to deprecate the local options.

Luckily this time it will be much easier since the value is just a string!