aaron.ballman added subscribers: erichkeane, tahonermann, ldionne,
hubert.reinterpretcast.
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1553-1558
+def err_static_assert_invalid_size : Error<
+ "the message in a static assertion must have a 'size()' member "
+ "function returning an object convertible to 'std::size_t'">;
+def err_static_assert_invalid_data : Error<
+ "the message in a static assertion must have a 'data()' member "
+ "function returning an object convertible to 'const char*'">;
----------------
How about we combine these into one diagnostic given how similar and related
they are?
================
Comment at: clang/lib/AST/ExprConstant.cpp:16417-16418
+ }
+ if (!Scope.destroy())
+ return false;
+
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Rather than use an RAII object and destroy it manually, let's use `{}` to
> > scope the RAII object appropriately.
> This seems to be the way to use this interface - mostly because destroy can
> fail and we want to detect that
Oh, what a strange thing to call `RAII`! :-D
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16888-16899
+ SourceLocation Loc = Message->getBeginLoc();
+ assert(Message);
+ assert(!Message->isTypeDependent() && !Message->isValueDependent() &&
+ "can't evaluate a dependant static assert message");
+
+ if (const auto *SL = dyn_cast<StringLiteral>(Message)) {
+ assert(SL->isUnevaluated() && "expected an unevaluated string");
----------------
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16902
+ if (!RD) {
+ Diag(Message->getBeginLoc(), diag::err_static_assert_invalid_message);
+ return false;
----------------
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16942-16943
+ if (SizeNotFound || DataNotFound) {
+ Diag(Message->getBeginLoc(),
+ diag::err_static_assert_missing_member_function)
+ << ((SizeNotFound && DataNotFound) ? 2
----------------
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16960
+ ExprResult Res = BuildMemberReferenceExpr(
+ Message, Message->getType(), Message->getBeginLoc(), false,
+ CXXScopeSpec(), SourceLocation(), nullptr, LR, nullptr, nullptr);
----------------
Can be re-flowed to 80 columns, and I'll stop pointing out changes for this one
-- just be sure to replace everything with `Loc` that you can.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16999-17000
+
+ if (CheckOnly)
+ return true;
+
----------------
Errrr, this seems like a pretty big surprise because evaluating that char range
can fail. This means:
```
constexpr struct {
size_t size() const;
const char *data() const;
} d;
static_assert(false, d); // Fails, not because of the static assertion but
because d is not valid
static_assert(true, d); // Succeeds, despite d not being valid
```
(Regardless of what we land on for a direction, I think this is a good test
case to add.)
I don't know that this is reasonable QoI. Most static assertions pass rather
than fail, so I think this will hide bugs from users in unexpected ways. I
understand there may be concerns about compile time overhead, but those
concerns seem misplaced in the absence of evidence: the extra overhead is
something the user is opting into explicitly by using constexpr features and
constexpr evaluation is well known to slow down compile times. Further, given
that many (most?) static assert messages are relatively short (e.g., not
kilobytes or megabytes of text), that extra overhead is likely negligible to
begin with in this case, at least on an individual assert basis. The downside
is, because most static assertions pass, that evaluation is also unnecessary in
most cases, and it could add up if there's a lot of static assertions. However,
given that users add static assertions to tell them when code is incorrect
(assumptions invalidated, etc), it seems pretty counter-productive to hide bugs
within the static assertion itself.
What do others think? @hubert.reinterpretcast @tahonermann @erichkeane @ldionne
@shafik
If there's some agreement that we'd like to see diagnostics here, my initial
preference is for an error diagnostic because a static assertion is only useful
if both operands are valid constant expressions, but I could probably live with
a warning. However, I think that an error would require filing a DR with WG21
otherwise there are conformance issues.
In the meantime, to keep this review moving without waiting for committee
deliberations, I think we can evaluate the constant expression to determine
it's validity and report issues as a warning (potentially one that defaults to
an error) if others agree that a diagnostic is warranted here.
================
Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:88-98
+struct ConvertibleToInt {
+ operator int();
+};
+struct ConvertibleToCharPtr {
+ operator const char*();
+};
+struct MessageFromConvertible {
----------------
Use of `true` here means that we missed the fact that
`MessageFromConvertible()` is invalid; not only are the `size` and `data`
methods not `constexpr`, the `ConvertibleToInt` and `ConvertibleToCharPtr`
classes do not have constexpr conversion operators. That could be intentional,
but we should definitely have a `false` test that tests the conversion
operators are properly called and work as expected when they're correct.
================
Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:110-111
+
+static_assert(false, Leaks{}); //expected-error {{the message in a static
assertion must be produced by a constant expression}} \
+ // expected-error {{static assertion failed: ub}}
+
----------------
This diagnostic combination is a little bit jarring to me. The first one is
telling the user that the message is incorrect and the second one tells the
user precisely what that message was, so they're almost contradictory in some
ways. However, I'm not certain if others think this is jarring as well.
If we think that, perhaps this sort of circumstance should just say the static
assertion failed without the message part, even though we could figure out
something as the message in this particular instance.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154290/new/
https://reviews.llvm.org/D154290
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
