aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16999-17000
+
+ if (CheckOnly)
+ return true;
+
----------------
tbaeder wrote:
> 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...
> Once they fail, it would be pretty disappointing if producing the error
> message itself would produce yet another error message...
That's a good way of restating why I don't think this is good QoI.
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
