[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D19769: [clang-tidy] Add explicitly given array size heuristic to misc-suspicious-missin
From:       Etienne Bergeron via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2016-04-30 17:54:44
Message-ID: 9cc40018902d1b253606571a4e95a17c () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]