[PATCH] D13640: [clang-tidy] Add new check cppcoreguidelines-pro-bounds-array-to-pointer-decay

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp:29
@@ +28,3 @@
+      implicitCastExpr(unless(hasParent(arraySubscriptExpr())),
+                       unless(hasSourceExpression(declRefExpr(to(varDecl(hasName("__range")))))),
+                       unless(hasSourceExpression(stringLiteral()))
----------------
mgehre wrote:
> sbenza wrote:
> > __range is an implementation detail and should not be used here.
> > Check that this is the range init of a cxxForRangeStmt parent node or something like that.
> That was my first attempt, but I couldn't quite figure it out.
> 
> I tried
>   unless(hasAncestor(cxxForRangeStmt(hasRangeInit(hasDescendant(equalsBoundNode("cast"))))))
> but that does not compile.
> 
> Any ideas?
I'm not certain that you'll be able to do that. Looking at the AST dump:

```
TranslationUnitDecl 0xa40168 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0xa40458 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list 'char *'
`-FunctionDecl 0xa404c0 <E:\Desktop\test1.cpp:1:1, line:5:1> line:1:6 f 'void (void)'
  `-CompoundStmt 0xa40b88 <col:10, line:5:1>
    |-DeclStmt 0xa40600 <line:2:3, col:11>
    | `-VarDecl 0xa405c8 <col:3, col:10> col:7 used a 'int [5]'
    `-CXXForRangeStmt 0xa40b50 <line:3:3, line:4:5>
      |-DeclStmt 0xa40828 <line:3:16>
      | `-VarDecl 0xa406b8 <col:16> col:16 implicit used __range 'int (&&)[5]' cinit
      |   `-DeclRefExpr 0xa40610 <col:16> 'int [5]' lvalue Var 0xa405c8 'a' 'int [5]'
      |-DeclStmt 0xa40a00 <col:14>
      | |-VarDecl 0xa40870 <col:14> col:14 implicit used __begin 'int *':'int *' cinit
      | | `-ImplicitCastExpr 0xa40960 <col:14> 'int *' <ArrayToPointerDecay>
      | |   `-DeclRefExpr 0xa40838 <col:14> 'int [5]' lvalue Var 0xa406b8 '__range' 'int (&&)[5]'
      | `-VarDecl 0xa408b0 <col:14, col:16> col:14 implicit used __end 'int *':'int *' cinit
      |   `-BinaryOperator 0xa409a0 <col:14, col:16> 'int *' '+'
      |     |-ImplicitCastExpr 0xa40990 <col:14> 'int *' <ArrayToPointerDecay>
      |     | `-DeclRefExpr 0xa40850 <col:14> 'int [5]' lvalue Var 0xa406b8 '__range' 'int (&&)[5]'
      |     `-IntegerLiteral 0xa40970 <col:16> 'int' 5
      |-BinaryOperator 0xa40a60 <col:14> '_Bool' '!='
      | |-ImplicitCastExpr 0xa40a40 <col:14> 'int *':'int *' <LValueToRValue>
      | | `-DeclRefExpr 0xa40a10 <col:14> 'int *':'int *' lvalue Var 0xa40870 '__begin' 'int *':'int *'
      | `-ImplicitCastExpr 0xa40a50 <col:14> 'int *':'int *' <LValueToRValue>
      |   `-DeclRefExpr 0xa40a28 <col:14> 'int *':'int *' lvalue Var 0xa408b0 '__end' 'int *':'int *'
      |-UnaryOperator 0xa40a90 <col:14> 'int *':'int *' lvalue prefix '++'
      | `-DeclRefExpr 0xa40a78 <col:14> 'int *':'int *' lvalue Var 0xa40870 '__begin' 'int *':'int *'
      |-DeclStmt 0xa40680 <col:7, col:18>
      | `-VarDecl 0xa40648 <col:7, col:14> col:12 e 'int':'int' cinit
      |   `-ImplicitCastExpr 0xa40b40 <col:14> 'int' <LValueToRValue>
      |     `-UnaryOperator 0xa40ad0 <col:14> 'int' lvalue prefix '*'
      |       `-ImplicitCastExpr 0xa40ac0 <col:14> 'int *':'int *' <LValueToRValue>
      |         `-DeclRefExpr 0xa40aa8 <col:14> 'int *':'int *' lvalue Var 0xa40870 '__begin' 'int *':'int *'
      `-NullStmt 0xa40b78 <line:4:5>
```
There is no range init in the AST. I'm not certain if that is an issue we want to rectify in the AST or not.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp:40
@@ +39,3 @@
+
+  diag(MatchedCast->getExprLoc(), "do not (implicitly) convert an array to a pointer");
+}
----------------
klimek wrote:
> Can't we provide a fixit?
I think the fixit that the C++ Core Guidelines wants is to use array_view, which isn't available to everyone and can't be applied locally (for instance, it may require changing a function parameter instead of the argument passed to the function).

================
Comment at: test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp:26
@@ +25,2 @@
+    return "clang"; // OK, decay string literal to pointer
+}
----------------
I would like to see a test case demonstrating that this does not trigger a diagnostic when creating an array_view object. We don't need to pull in all of the array_view machinery from GSL, but it would be good to have a skeleton of the implementation to ensure we aren't diagnosing there.


http://reviews.llvm.org/D13640