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

[Dominik Szabó via cfe-commits]

szdominik added a comment.

I think the main success is that we don't warn here:

  const char* A[2] = {"a", "b" "c");

The problem is, as you said, not remove already supported cases (such as when 
the parent is not a vardecl (but also an initListExpr)).
But I'll implement, and we'll see it.


http://reviews.llvm.org/D19769



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

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

[Dominik Szabó via cfe-commits]

szdominik added a comment.

The original size is available - but from the decleration, not from the 
initListExpr.



Comment at: clang-tidy/misc/SuspiciousMissingCommaCheck.cpp:106
@@ +105,3 @@
+  if (InitializerList->hasArrayFiller()) {
+  diag(InitializerList->getExprLoc(),
+   "wrong string array initialization: "

etienneb wrote:
> szdominik wrote:
> > etienneb wrote:
> > > The error should still be reported to the missing comma (concatenated 
> > > token):
> > >   ConcatenatedLiteral->getLocStart(),
> > > 
> > > We could add a NOTE to point to the array, stating that the size mismatch.
> > > 
> > > What do you think?
> > Interesting question (the first idea was that we can't decide that the 
> > comma is missing or the size is wrong, so report to the array, that's a 
> > more secure solution), but I agree that the note could be more effective.
> > And... it's still a suspicious-missing-comma checker, not a 
> > wrong-string-array-size checker :)
> How can you be sure the size was provided by the user? And not inferred by 
> the type system?
> 
> For the following examples:
> ```
> const char* listA[] = {"a", "b" };
> const char* listB[5] = {"a", "b" };
> ```
> 
> We've got this:
> ```
> VarDecl 0x5e9d840  col:13 
> listA 'const char *[2]' cinit
> `-InitListExpr 0x5e9d950  'const char *[2]'
>   |-ImplicitCastExpr 0x5e9d978  'const char *' 
>   | `-StringLiteral 0x5e9d8d8  'const char [2]' lvalue "a"
>   `-ImplicitCastExpr 0x5e9d988  'const char *' 
> `-StringLiteral 0x5e9d8fc  'const char [2]' lvalue "b"
> ```
> 
> ```
> VarDecl 0x5e9d840  col:13 
> listA 'const char *[2]' cinit
> `-InitListExpr 0x5e9d950  'const char *[2]'
>   |-ImplicitCastExpr 0x5e9d978  'const char *' 
>   | `-StringLiteral 0x5e9d8d8  'const char [2]' lvalue "a"
>   `-ImplicitCastExpr 0x5e9d988  'const char *' 
> `-StringLiteral 0x5e9d8fc  'const char [2]' lvalue "b"
> ```
> 
> How can I tell the "size" was written by the user?
> How can you get the "5" and not the "2".
> 
(You copied twice the AST-tree part of listA :))

The array, which was declared with explicit given size, has different type than 
the usual one. And I can filter the proper cases because of the different 
(incomplete / constant) array type. 
e.g.

```
VariableDeclaration->getTypeSourceInfo()->getType()->dump()

```
```
const char *string_array_init[3] = {
"first elem",
"second elem"
"third elem"
};
```
```
ConstantArrayType 0x1efae70 'const char *[3]' 3 
`-PointerType 0x1efae40 'const char *'
  `-QualType 0x1efa511 'const char' const
`-BuiltinType 0x1efa510 'char'
```
But
```
const char *string_array_init2[] = {
"first elem",
"second elem"
"third elem"
};
```
```
IncompleteArrayType 0x1f49ba0 'const char *[]' 
`-PointerType 0x1efae40 'const char *'
  `-QualType 0x1efa511 'const char' const
`-BuiltinType 0x1efa510 'char'
```
However, the type of InitListExpr is always ConstantArrayType (I tried to find 
a way to be sure the size is explicit given or inferred, but... I don't find 
anything).
So (unless you haven't better idea) I have two options, as I see:
a) use the array filler solution, but I can't reduce the false-positive cases.
b) use the decleration's type, but I have to use some kind of heuristic, what 
you told me at your first comment.
Probably we should decide which one is less used.


http://reviews.llvm.org/D19769



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

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

[Dominik Szabó via cfe-commits]

szdominik marked 3 inline comments as done.


Comment at: clang-tidy/misc/SuspiciousMissingCommaCheck.cpp:106
@@ +105,3 @@
+  if (InitializerList->hasArrayFiller()) {
+  diag(InitializerList->getExprLoc(),
+   "wrong string array initialization: "

etienneb wrote:
> The error should still be reported to the missing comma (concatenated token):
>   ConcatenatedLiteral->getLocStart(),
> 
> We could add a NOTE to point to the array, stating that the size mismatch.
> 
> What do you think?
Interesting question (the first idea was that we can't decide that the comma is 
missing or the size is wrong, so report to the array, that's a more secure 
solution), but I agree that the note could be more effective.
And... it's still a suspicious-missing-comma checker, not a 
wrong-string-array-size checker :)


http://reviews.llvm.org/D19769



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

Re: [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:94
@@ -93,3 +93,3 @@
   Finder->addMatcher(StringsInitializerList.bind("list"), this);
 }
 

etienneb wrote:
> If it's working as-is,... this is neat  :)
Well, the array filler has a problematic limitation.
I forget that I used the explicit given size for reduce false positives in my 
checker.
e.g.
```
struct MusicIntervalArray {
int n;
const char* list[5];
} intervals[2] = {
{5, {"second", "third", "fourth", "fifth" "sixth"}},
{5, {"ninth", "tenth", "eleventh", "twelfth", "thir" "teenth"}},
};
```
The first is simple: has array filler, so warn because of size.
But the second one is a good initialization, it has 5 elements, which is the 
explicit size. But your implementation warns us, that there is a missing comma. 
If I work with the length (from declaration or type), I can filter these cases.


http://reviews.llvm.org/D19769



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

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

[Dominik Szabó via cfe-commits]

szdominik updated this revision to Diff 55741.
szdominik added a comment.

Implement the heuristic in a different (and simpler) way.
Based on the array fillers.


http://reviews.llvm.org/D19769

Files:
  clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
  docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
  test/clang-tidy/misc-suspicious-missing-comma.cpp

Index: test/clang-tidy/misc-suspicious-missing-comma.cpp
===
--- test/clang-tidy/misc-suspicious-missing-comma.cpp
+++ test/clang-tidy/misc-suspicious-missing-comma.cpp
@@ -80,3 +80,21 @@
   "Dummy line",
   "Dummy line",
 };
+
+// Missing comma or wrong explicit array size.
+const char* TheThreeMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis"
+  "D'Artagnan"
+};
+// CHECK-MESSAGES: :[[@LINE-6]]:3: warning: wrong string array initialization: 
the explicit given size and the real number of elements aren't equal 
[misc-suspicious-missing-comma]
+
+// Correctly given array size should avoid warning.
+const char* TheFourMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis",
+  "Charles de Batz de Castelmore"
+  "D'Artagnan"
+};
Index: docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
===
--- docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
+++ docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
@@ -42,3 +42,14 @@
 "Warning %s",
   };
 
+This checker is also capable of warn on cases when the explicitly given array 
size
+isn't equal to the real array size.
+
+.. code:: c++
+
+  const char* TheThreeMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis"
+  "D'Artagnan"
+};
Index: clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
===
--- clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
+++ clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
@@ -100,6 +100,16 @@
   Result.Nodes.getNodeAs("str");
   assert(InitializerList && ConcatenatedLiteral);
   
+  // InitializerList has an array filler when the explicitly given size is
+  // bigger than the real array size - e.g. because there is a missing comma.
+  if (InitializerList->hasArrayFiller()) {
+  diag(InitializerList->getExprLoc(),
+   "wrong string array initialization: "
+   "the explicit given size and the "
+   "real number of elements aren't equal");
+  return;
+  }
+
   // Skip small arrays as they often generate false-positive.
   unsigned int Size = InitializerList->getNumInits();
   if (Size < SizeThreshold) return;


Index: test/clang-tidy/misc-suspicious-missing-comma.cpp
===
--- test/clang-tidy/misc-suspicious-missing-comma.cpp
+++ test/clang-tidy/misc-suspicious-missing-comma.cpp
@@ -80,3 +80,21 @@
   "Dummy line",
   "Dummy line",
 };
+
+// Missing comma or wrong explicit array size.
+const char* TheThreeMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis"
+  "D'Artagnan"
+};
+// CHECK-MESSAGES: :[[@LINE-6]]:3: warning: wrong string array initialization: the explicit given size and the real number of elements aren't equal [misc-suspicious-missing-comma]
+
+// Correctly given array size should avoid warning.
+const char* TheFourMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis",
+  "Charles de Batz de Castelmore"
+  "D'Artagnan"
+};
Index: docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
===
--- docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
+++ docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
@@ -42,3 +42,14 @@
 "Warning %s",
   };
 
+This checker is also capable of warn on cases when the explicitly given array size
+isn't equal to the real array size.
+
+.. code:: c++
+
+  const char* TheThreeMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis"
+  "D'Artagnan"
+};
Index: clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
===
--- clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
+++ clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
@@ -100,6 +100,16 @@
   Result.Nodes.getNodeAs("str");
   assert(InitializerList && ConcatenatedLiteral);
   
+  // InitializerList has an array filler when the explicitly given size is
+  // bigger than the real array size - e.g. because there is a missing comma.
+  if (InitializerList->hasArrayFiller()) {
+  diag(InitializerList->getExprLoc(),
+   "wrong string array initialization: "
+   "the explicit given size and the "
+   "real number of elements aren't equal");
+  return;
+  }
+
   // Skip small arrays as they often generate false-positive.
   unsigned int Size = InitializerList->getNumInits();
   if (Size < SizeThreshold) return;
___
cfe-commits mailing list

Re: [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  line:9:1> line:6:3 A 'struct StrArray [2]' cinit
> `-InitListExpr 0x1e82c38  'struct StrArray [2]'
>   |-InitListExpr 0x1e82c88  'struct StrArray':'struct 
> StrArray'
>   | |-IntegerLiteral 0x1e827d8  'int' 5
>   | `-InitListExpr 0x1e82cd8  'const char *[5]'
>   |   |-ImplicitCastExpr 0x1e82d40  'const char *' 
> 
>   |   | `-StringLiteral 0x1e82878  'const char [2]' lvalue "a"
>   |   |-ImplicitCastExpr 0x1e82d58  'const char *' 
> 
>   |   | `-StringLiteral 0x1e828a8  'const char [2]' lvalue "b"
>   |   |-ImplicitCastExpr 0x1e82d70  'const char *' 
> 
>   |   | `-StringLiteral 0x1e828d8  'const char [2]' lvalue "c"
>   |   |-ImplicitCastExpr 0x1e82d88  'const char *' 
> 
>   |   | `-StringLiteral 0x1e82908  'const char [2]' lvalue "d"
>   |   `-ImplicitCastExpr 0x1e82da0  'const char *' 
> 
>   | `-StringLiteral 0x1e82938  'const char [2]' lvalue "e"
>   `-InitListExpr 0x1e82db8  'struct StrArray':'struct 
> StrArray'
> |-IntegerLiteral 0x1e82a20  'int' 5
> `-InitListExpr 0x1e82e08  'const char *[5]'
>   |-ImplicitCastExpr 0x1e82e70  'const char *' 
> 
>   | `-StringLiteral 0x1e82a40  'const char [2]' lvalue "a"
>   |-ImplicitCastExpr 0x1e82e88  'const char *' 
> 
>   | `-StringLiteral 0x1e82a70  'const char [2]' lvalue "b"
>   |-ImplicitCastExpr 0x1e82ea0  'const char *' 
> 
>   | `-StringLiteral 0x1e82aa0  'const char [2]' lvalue "c"
>   |-ImplicitCastExpr 0x1e82eb8  'const char *' 
> 
>   | `-StringLiteral 0x1e82ad0  'const char [2]' lvalue "d"
>   `-ImplicitCastExpr 0x1e82ed0  'const char *' 
> 
> `-StringLiteral 0x1e82b00  '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  'struct StrArray':'struct 
> StrArray'
> |-IntegerLiteral 0x28ff9e8  'int' 5
> `-InitListExpr 0x28ffda0  'const char *[5]'
>   |-array filler
>   | `-ImplicitValueInitExpr 0x28ffe78 <> 'const char *'
>   |-ImplicitCastExpr 0x28ffde0  'const char *' 
> 
>   | `-StringLiteral 0x28ffa08  'const char [2]' lvalue "a"
>   |-ImplicitCastExpr 0x28ffe00  'const char *' 
> 
>   | `-StringLiteral 0x28ffa38  'const char [2]' lvalue "b"
>   |-ImplicitCastExpr 0x28ffe28  'const char *' 
> 
>   | `-StringLiteral 0x28ffa68  'const char [2]' lvalue "c"
>   `-ImplicitCastExpr 0x28ffe60  'const char *' 
> 
> `-StringLiteral 0x28ffa98  'const char [2]' lvalue "d"
> ```
> 
> For the direct case:
> ```
> const char* list[5] = { "a", "b"};
> ```
> 
> It has the following AST-representation:
> 
> ```
> VarDecl 0x2c67f00  col:33> col:13 list 'const char *[5]' cinit
> `-InitListExpr 0x2c68010  'const char *[5]'
>   |-array filler
>   | `-ImplicitValueInitExpr 0x2c68098 <> 'const char *'
>   |-ImplicitCastExpr 0x2c68050  'const char *' 
>   | `-StringLiteral 0x2c67f60  'const char [2]' lvalue "a"
>   `-ImplicitCastExpr 0x2c68070  'const char *' 
> `-StringLiteral 0x2c67f90  '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

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

[Dominik Szabó via cfe-commits]

szdominik created this revision.
szdominik added reviewers: alexfh, etienneb.
szdominik added subscribers: cfe-commits, xazax.hun.

Additional heuristic to misc-suspicious-missing-comma checker, based on the 
(in)equality of the explicitly given array size and the real array size.
Note: in these cases we don't know that the given size is wrong or there is a 
missing comma.

Original checker revision: http://reviews.llvm.org/D18457

http://reviews.llvm.org/D19769

Files:
  clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
  docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
  test/clang-tidy/misc-suspicious-missing-comma.cpp

Index: test/clang-tidy/misc-suspicious-missing-comma.cpp
===
--- test/clang-tidy/misc-suspicious-missing-comma.cpp
+++ test/clang-tidy/misc-suspicious-missing-comma.cpp
@@ -80,3 +80,21 @@
   "Dummy line",
   "Dummy line",
 };
+
+// Missing comma or wrong explicit array size.
+const char* TheThreeMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis"
+  "D'Artagnan"
+};
+// CHECK-MESSAGES: :[[@LINE-6]]:3: warning: wrong string array initialization: 
the explicit given size and the real number of elements aren't equal 
[misc-suspicious-missing-comma]
+
+// Correctly given array size should avoid warning.
+const char* TheFourMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis",
+  "Charles de Batz de Castelmore"
+  "D'Artagnan"
+};
Index: docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
===
--- docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
+++ docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
@@ -42,3 +42,14 @@
 "Warning %s",
   };
 
+This checker is also capable of warn on cases when the explicitly given array 
size
+isn't equal to the real array size.
+
+.. code:: c++
+
+  const char* TheThreeMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis"
+  "D'Artagnan"
+};
Index: clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
===
--- clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
+++ clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
@@ -86,9 +86,11 @@
   const auto ConcatenatedStringLiteral =
   stringLiteral(isConcatenatedLiteral(MaxConcatenatedTokens)).bind("str");
 
-  const auto StringsInitializerList =
-  initListExpr(hasType(constantArrayType()),
-   has(expr(ignoringImpCasts(ConcatenatedStringLiteral;
+  const auto StringsInitializerList = initListExpr(
+  hasType(constantArrayType()),
+  has(expr(ignoringImpCasts(ConcatenatedStringLiteral))),
+  hasParent(varDecl(allOf(hasType(arrayType()), isDefinition()))
+.bind("varDecl")));
 
   Finder->addMatcher(StringsInitializerList.bind("list"), this);
 }
@@ -98,10 +100,29 @@
   const auto *InitializerList = Result.Nodes.getNodeAs("list");
   const auto *ConcatenatedLiteral =
   Result.Nodes.getNodeAs("str");
-  assert(InitializerList && ConcatenatedLiteral);
-  
-  // Skip small arrays as they often generate false-positive.
+  const auto *VariableDeclaration = Result.Nodes.getNodeAs("varDecl");
+  assert(InitializerList && ConcatenatedLiteral && VariableDeclaration);
+
   unsigned int Size = InitializerList->getNumInits();
+
+  // Warn when the explicit given array size isn't equal to the real array 
size.
+  QualType DeclType = VariableDeclaration->getTypeSourceInfo()->getType();
+  if (DeclType->isConstantArrayType()) {
+unsigned int ExplicitArraySize = 0;
+ExplicitArraySize =
+((Result.Context)->getAsConstantArrayType(DeclType)->getSize())
+.getLimitedValue();
+
+if (Size != ExplicitArraySize) {
+  diag(InitializerList->getExprLoc(),
+   "wrong string array initialization: "
+   "the explicit given size and the "
+   "real number of elements aren't equal");
+  return;
+}
+  }
+
+  // Skip small arrays as they often generate false-positive.
   if (Size < SizeThreshold) return;
 
   // Count the number of occurence of concatenated string literal.


Index: test/clang-tidy/misc-suspicious-missing-comma.cpp
===
--- test/clang-tidy/misc-suspicious-missing-comma.cpp
+++ test/clang-tidy/misc-suspicious-missing-comma.cpp
@@ -80,3 +80,21 @@
   "Dummy line",
   "Dummy line",
 };
+
+// Missing comma or wrong explicit array size.
+const char* TheThreeMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis"
+  "D'Artagnan"
+};
+// CHECK-MESSAGES: :[[@LINE-6]]:3: warning: wrong string array initialization: the explicit given size and the real number of elements aren't equal [misc-suspicious-missing-comma]
+
+// Correctly given array size should avoid warning.
+const char* TheFourMusketeers[4] = {
+  "Athos",
+  "Porthos",
+  "Aramis",
+  "Charles de Batz de Castelmore"
+  "D'Artagnan"
+};
Index: 

Re: [PATCH] D18457: [clang-tidy] Add a new checker to detect missing comma in initializer list.

[Dominik Szabó via cfe-commits]

szdominik added a comment.

Hi!

I'm working on the same (or almost the same) checker as you, so maybe you'll 
find helpful my code.
I think your approach (or heuristics) is better than mine, but there are other 
options in this problem.
e.g.

- more precise location (where exactly is the missing comma)
- FixItHint for that
- think about what if the explicit array size is given (see below in my test)

Please, ask me if you have any question about my checker.

F1716939: misc-wrong-string-array-initialization.cpp 
 (test file)
F1716926: WrongStringArrayInitializationCheck.h 

F1716927: WrongStringArrayInitializationCheck.cpp 



http://reviews.llvm.org/D18457



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