[PATCH] D19769: [clang-tidy] Add explicitly given array size heuristic to misc-suspicious-missing-comma check.
Dominik Szabó via cfe-commits
cfe-commits at lists.llvm.org
Sun May 1 01:44:22 PDT 2016
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
More information about the cfe-commits
mailing list