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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
