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

[Etienne Bergeron via cfe-commits]

etienneb added a comment.

If you are interested to a quick chat on this Checker, ping me. I know other cases that should be filtered and I'm lacking time to implement them. Here is a common one I would lie to see happening (base on comments):

  const char* A[] = {
    // This is a entry
    "entry1",
    // This is the next entry
    "entry2"
    "entry2"
    "entry2",
    // This is the last entry
    "entry2",
  };

Other common cases to append literals are:

  "Program version:"  VERSION 
  "Program version: %"  PRIu64 
  "Let add strange characters \xFF" "For nothing"  (you can't happend these two literals or the escaped char will be "\xFFF")


================
Comment at: clang-tidy/misc/SuspiciousMissingCommaCheck.cpp:92
@@ +91,3 @@
+      has(expr(ignoringImpCasts(ConcatenatedStringLiteral))),
+      hasParent(varDecl(allOf(hasType(arrayType()), isDefinition()))
+                    .bind("varDecl")));
----------------
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.




http://reviews.llvm.org/D19769