[PATCH] D154290: [Clang] Implement P2741R3 - user-generated static_assert messages

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is looking pretty close to good, but there were some outstanding questions still.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:86
   "%select{case value|enumerator value|non-type template argument|"
-  "array size|explicit specifier argument|noexcept specifier argument}0 "
+  "array size|explicit specifier argument|noexcept specifier argument|call to size()|call to data()}0 "
   "is not a constant expression">;
----------------
aaron.ballman wrote:
> This should also be re-flowed to 80 columns.
It looks like these edits were missed.


================
Comment at: clang/lib/AST/DeclPrinter.cpp:952-953
                                   &Context);
-  if (StringLiteral *SL = D->getMessage()) {
+  if (StringLiteral *SL =
+          llvm::dyn_cast_if_present<StringLiteral>(D->getMessage())) {
     Out << ", ";
----------------
Should there be an `else` clause here that does `D->getMessage()->prettyPrint()` so that we print out *something*? Otherwise, there should be a fixme comment about not printing out non-string literal operands.


================
Comment at: clang/lib/AST/ExprConstant.cpp:16408
+                                        Char) ||
+        !Char.isInt())
+      return false;
----------------
shafik wrote:
> Why are we specifically checking `!isInt()` what `Kind` do we expect?
I think the caller already validates that we have a pointer to the correct kind of string, so I kind of think the `isInt()` check is unnecessary. If it's not, can you show a test case where it kicks in?


================
Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:269
+                           //expected-error {{static assertion failed}} \
+                           // expected-note {{in call to 'InvalidPtr{}.data()'}}
----------------
One more test case to consider: do we properly handle dependent expressions in the assert itself? e.g.,
```
template <typename Ty>
struct S {
  static_assert(false, Ty{});
};

struct Frobble {
  constexpr int size() const { return 5; }
  consterxpr const char *data() const { return "hello"; }
};

S<Frobble> s; // Good, fails with "hello"
S<int> t; // Incorrect
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154290