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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

(Not a full review, I ran out of steam -- I wanted to get you some feedback that I already found though.)



================
Comment at: clang/include/clang/AST/Expr.h:766-767
+  bool EvaluateCharPointerAsString(std::string &Result,
+                                   const Expr *SizeExpression,
+                                   const Expr *PtrExpression, ASTContext &Ctx,
+                                   EvalResult &Status) const;
----------------
The function name confuses me a bit because a char pointer doesn't have a size expression. I was thinking "EvaluateCharArrayAsString" but that's also not right.


================
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">;
----------------
This should also be re-flowed to 80 columns.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1545-1546
   "expression evaluates to '%0 %1 %2'">;
+def err_static_assert_invalid_message : Error<"the message in a static_assert "
+  "declaration must be a string literal or an object with data() and size() member functions">;
+def err_static_assert_invalid_size : Error<"the message in a static_assert declaration "
----------------
And re-flow to 80 columns. You should make similar changes in the other new diagnostics as well. (Use single quotes around syntax elements, start the string on its own line rather inline with the `def`, use "static assertion" instead of "static_assert" (The last point is largely because C has both _Static_assert and static_assert and we're avoiding figuring out which one was used. It may be moot as this is C++-only functionality, but it is more consistent with other static assert diagnostics.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:16417-16418
+  }
+  if (!Scope.destroy())
+    return false;
+
----------------
Rather than use an RAII object and destroy it manually, let's use `{}` to scope the RAII object appropriately.


================
Comment at: clang/lib/AST/ExprConstant.cpp:16413
+    APSInt C = Char.getInt();
+    Result.push_back(static_cast<char>(C.getExtValue()));
+    if (!HandleLValueArrayAdjustment(Info, PtrExpression, String, CharTy, 1))
----------------
barannikov88 wrote:
> This relies on host's CHAR_BIT >= target's CHAR_BIT, which isn't true for my target. Could you add an assertion?
> 
Wouldn't adding the assertion cause you problems then? (FWIW, we only support `CHAR_BIT == 8` currently.)


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16922-16927
+  std::optional<LookupResult> SizeMember = FindMember("size");
+  std::optional<LookupResult> DataMember = FindMember("data");
+  if (!SizeMember || !DataMember) {
+    Diag(Message->getBeginLoc(), diag::err_static_assert_invalid_message);
+    return false;
+  }
----------------
It might be more friendly to tell the user which was missing, `size` or `data`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154290