[PATCH] D19769: [clang-tidy] Add explicitly given array size heuristic to misc-suspicious-missing-comma check.

[Dominik Szabó via cfe-commits]

szdominik added inline comments.

================
Comment at: clang-tidy/misc/SuspiciousMissingCommaCheck.cpp:92
@@ +91,3 @@
+      has(expr(ignoringImpCasts(ConcatenatedStringLiteral))),
+      hasParent(varDecl(allOf(hasType(arrayType()), isDefinition()))
+                    .bind("varDecl")));
----------------
etienneb wrote:
> The issue I see by matching the "hasParent" that way is that you are assuming that the matcher is only looking for initialisation-list attached to variable declaration (direct child).
> 
> Assume this case:
> ```
> struct StrArray {
>   int n;
>   const char* list[5];
> } A[2] = {
>   {5, {"a", "b", "c", "d", "e" }},
>   {5, {"a", "b", "c", "d", "e" }},
> };
> ```
> 
> It is giving you this AST representation:
> ```
> VarDecl 0x1e82738 </usr/local/google/home/etienneb/examples/test.cc:3:1, line:9:1> line:6:3 A 'struct StrArray [2]' cinit
> `-InitListExpr 0x1e82c38 <col:10, line:9:1> 'struct StrArray [2]'
>   |-InitListExpr 0x1e82c88 <line:7:3, col:33> 'struct StrArray':'struct StrArray'
>   | |-IntegerLiteral 0x1e827d8 <col:4> 'int' 5
>   | `-InitListExpr 0x1e82cd8 <col:7, col:32> 'const char *[5]'
>   |   |-ImplicitCastExpr 0x1e82d40 <col:8> 'const char *' <ArrayToPointerDecay>
>   |   | `-StringLiteral 0x1e82878 <col:8> 'const char [2]' lvalue "a"
>   |   |-ImplicitCastExpr 0x1e82d58 <col:13> 'const char *' <ArrayToPointerDecay>
>   |   | `-StringLiteral 0x1e828a8 <col:13> 'const char [2]' lvalue "b"
>   |   |-ImplicitCastExpr 0x1e82d70 <col:18> 'const char *' <ArrayToPointerDecay>
>   |   | `-StringLiteral 0x1e828d8 <col:18> 'const char [2]' lvalue "c"
>   |   |-ImplicitCastExpr 0x1e82d88 <col:23> 'const char *' <ArrayToPointerDecay>
>   |   | `-StringLiteral 0x1e82908 <col:23> 'const char [2]' lvalue "d"
>   |   `-ImplicitCastExpr 0x1e82da0 <col:28> 'const char *' <ArrayToPointerDecay>
>   |     `-StringLiteral 0x1e82938 <col:28> 'const char [2]' lvalue "e"
>   `-InitListExpr 0x1e82db8 <line:8:3, col:33> 'struct StrArray':'struct StrArray'
>     |-IntegerLiteral 0x1e82a20 <col:4> 'int' 5
>     `-InitListExpr 0x1e82e08 <col:7, col:32> 'const char *[5]'
>       |-ImplicitCastExpr 0x1e82e70 <col:8> 'const char *' <ArrayToPointerDecay>
>       | `-StringLiteral 0x1e82a40 <col:8> 'const char [2]' lvalue "a"
>       |-ImplicitCastExpr 0x1e82e88 <col:13> 'const char *' <ArrayToPointerDecay>
>       | `-StringLiteral 0x1e82a70 <col:13> 'const char [2]' lvalue "b"
>       |-ImplicitCastExpr 0x1e82ea0 <col:18> 'const char *' <ArrayToPointerDecay>
>       | `-StringLiteral 0x1e82aa0 <col:18> 'const char [2]' lvalue "c"
>       |-ImplicitCastExpr 0x1e82eb8 <col:23> 'const char *' <ArrayToPointerDecay>
>       | `-StringLiteral 0x1e82ad0 <col:23> 'const char [2]' lvalue "d"
>       `-ImplicitCastExpr 0x1e82ed0 <col:28> 'const char *' <ArrayToPointerDecay>
>         `-StringLiteral 0x1e82b00 <col:28> 'const char [2]' lvalue "e"
> ```
> 
> I believe this can be solved with something like:
>       hasParent(anyOf(varDecl(allOf(hasType(arrayType()), isDefinition()),
>                                   anything()))  <<-- to accept all other cases.
> 
> That way, you are adding an heuristic to filter some incorrect warnings.
> 
> I believe it's possible to match the example above as the size is part of the type.
> 
> If I try this example:
> 
> ```
> {5, {"a", "b", "c", "d"}},    // only four string literal
> ```
> 
> I'm getting the following AST:
> ```
>  `-InitListExpr 0x28ffd50 <line:8:3, col:27> 'struct StrArray':'struct StrArray'
>     |-IntegerLiteral 0x28ff9e8 <col:4> 'int' 5
>     `-InitListExpr 0x28ffda0 <col:7, col:26> 'const char *[5]'
>       |-array filler
>       | `-ImplicitValueInitExpr 0x28ffe78 <<invalid sloc>> 'const char *'
>       |-ImplicitCastExpr 0x28ffde0 <col:8> 'const char *' <ArrayToPointerDecay>
>       | `-StringLiteral 0x28ffa08 <col:8> 'const char [2]' lvalue "a"
>       |-ImplicitCastExpr 0x28ffe00 <col:13> 'const char *' <ArrayToPointerDecay>
>       | `-StringLiteral 0x28ffa38 <col:13> 'const char [2]' lvalue "b"
>       |-ImplicitCastExpr 0x28ffe28 <col:18> 'const char *' <ArrayToPointerDecay>
>       | `-StringLiteral 0x28ffa68 <col:18> 'const char [2]' lvalue "c"
>       `-ImplicitCastExpr 0x28ffe60 <col:23> 'const char *' <ArrayToPointerDecay>
>         `-StringLiteral 0x28ffa98 <col:23> 'const char [2]' lvalue "d"
> ```
> 
> For the direct case:
> ```
> const char* list[5] = { "a", "b"};
> ```
> 
> It has the following AST-representation:
> 
> ```
> VarDecl 0x2c67f00 </usr/local/google/home/etienneb/examples/test.cc:11:1, col:33> col:13 list 'const char *[5]' cinit
> `-InitListExpr 0x2c68010 <col:23, col:33> 'const char *[5]'
>   |-array filler
>   | `-ImplicitValueInitExpr 0x2c68098 <<invalid sloc>> 'const char *'
>   |-ImplicitCastExpr 0x2c68050 <col:25> 'const char *' <ArrayToPointerDecay>
>   | `-StringLiteral 0x2c67f60 <col:25> 'const char [2]' lvalue "a"
>   `-ImplicitCastExpr 0x2c68070 <col:30> 'const char *' <ArrayToPointerDecay>
>     `-StringLiteral 0x2c67f90 <col:30> 'const char [2]' lvalue "b"
> ```
> So, it seems the length could be taken from the type instead of the declaration?!
> Or, look at what is the "Array Filler" (I still don't know). This may be an more straight forward way.
> 
> 
The array filler could be our solution.
I didn't find too much description about it, but based on this:
http://clang.llvm.org/doxygen/Expr_8cpp_source.html#l01963
I guess, there is always an array filler in these cases (when the explicit size is bigger than the real, so when there is a "hole" in the array because of the size-diff).
It's more simplier way, you're right.


http://reviews.llvm.org/D19769