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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16999-17000
+
+  if (CheckOnly)
+    return true;
+
----------------
aaron.ballman wrote:
> 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.
> 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.

It might be true that they usually pass, but as assertions, their purpose is to eventually fail. Just like the runtime assertions in clang usually pass but also fail all the time (see the bug tracker). Once they fail, it would be pretty disappointing if producing the error message itself would produce yet another error message...


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17009-17010
+    Diag(Message->getBeginLoc(), diag::err_static_assert_message_constexpr);
+    for (unsigned I = 0; I < Notes.size(); ++I)
+      Diag(Notes[I].first, Notes[I].second);
+    return false;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154290