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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits