[PATCH] D60561: [clang] fixing diagnostics of constexpr callstack

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:486
+    /// diagnostics
+    ConstArgs ConstantArgs;
+
----------------
Please don't use a typedef for this. Follow the style of the surrounding lines.

    const APValue *ConstantArgs;

(The pointer member itself should not be `const`-qualified. Compare to `const LValue *This;` two declarations above.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:4520
                                   APValue *ArgValues,
+                                  APValue const *const ConstantArgs,
                                   const CXXConstructorDecl *Definition,
----------------
`const APValue *ConstantArgs,`


================
Comment at: clang/lib/AST/ExprConstant.cpp:4684
 
-  return HandleConstructorCall(E, This, ArgValues.data(), Definition,
-                               Info, Result);
+  ArgVector ConstantArg(ArgValues.begin(), ArgValues.end());
+  return HandleConstructorCall(E, This, ArgValues.data(), ConstantArg.data(),
----------------
Surely you meant `ConstantArgs` (plural) here.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx1y.cpp:1183
+// expected-note at +1 {{'f2(0)}}
+constexpr int a = f2(0);
----------------
Personally, I would prefer to see something as simple as my original example; I don't think the extra 6 levels of recursion here makes the test any easier to understand.
```
constexpr int f(int i) {
    i = -i;
    return 1 << i;  // expected-note{{negative shift count -1}}
}
static_assert(f(1));
// expected-error at -1{{static_assert expression is not an integral constant expression}}
// expected-note at -2{{in call to 'f(1)'}}
```

Also, procedural note: I expect you'll be asked to reupload this diff with full context (`git diff -U999 ...`).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60561/new/

https://reviews.llvm.org/D60561