Quuxplusone added a subscriber: saar.raz.
Quuxplusone added inline comments.
================
Comment at: test/PCH/cxx-static_assert.cpp:17
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is
not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>'
requested here}}
----------------
aaron.ballman wrote:
> Quuxplusone wrote:
> > courbet wrote:
> > > aaron.ballman wrote:
> > > > I'm not certain how I feel about now printing the failure condition
> > > > when there's an explicit message provided. From what I understand, a
> > > > fair amount of code in the wild does `static_assert(some_condition,
> > > > "some_condition")` because of older language modes where the message
> > > > was not optional. I worry we're going to start seeing a lot of
> > > > diagnostics like: `static_assert failed due to requirement '1 == 2' "N
> > > > == 2"`, which seems a bit low-quality. See
> > > > `DFAPacketizer::DFAPacketizer()` in LLVM as an example of something
> > > > similar.
> > > >
> > > > Given that the user entered a message, do we still want to show the
> > > > requirement? Do we feel the same way if the requirement is fairly large?
> > > The issue is that `"N == 2"` is a useless error message. Actually, since
> > > the error message has to be a string literal, there is no way for the
> > > user to print a debuggable output. So I really think we should print the
> > > failed condition.
> > FWIW, I also don't agree with Aaron's concern.
> >
> > I do think there is a lot of code in the wild whose string literal was
> > "phoned in." After all, this is why C++17 allows us to omit the string
> > literal: to avoid boilerplate like this.
> >
> > static_assert(some-condition, "some-condition");
> > static_assert(some-condition, "some-condition was not satisfied");
> > static_assert(some-condition, "some-condition must be satisfied");
> > static_assert(some-condition, "");
> >
> > But should Clang go _out of its way_ to suppress such "redundant" string
> > literals? First of all, such suppression would be C++14-and-earlier: if a
> > C++17-native program contains a string literal, we should maybe assume it's
> > on purpose. Second, it's not clear how a machine could detect "redundant"
> > literals with high fidelity.
> >
> > static_assert(std::is_integral<T>, "std::is_integral<T>");
> > // clearly redundant
> > static_assert(std::is_integral<T>, "T must be integral");
> > // redundant, but unlikely to be machine-detectable as such
> >
> > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> > sizeof(DFAInput)),
> > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 * sizeof(DFAInput))");
> > // redundant, but unlikely to be machine-detectable as such
> > // thanks to the substitution of > for <=
> >
> > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> > sizeof(DFAInput)),
> > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
> > // redundant, but unlikely to be machine-detectable as such
> >
> > In any event, I agree with @courbet that Clang should print the expansion
> > of the failed condition in any case.
> >
> > Also: One might argue that if the `static_assert` fits completely on one
> > source line, then we could omit the string-literal from our error message
> > because the string literal will be shown anyway as part of the offending
> > source line — but I believe IDE-users would complain about that, so, we
> > shouldn't do that. And even then, we'd still want to print the failed
> > condition!
> >
> > Checking my understanding: The `static_assert` above (taken from
> > `DFAPacketizer::DFAPacketizer()` in LLVM) would be unchanged by @courbet's
> > patches, because none of its subexpressions are template-dependent. Right?
> > But should Clang go _out of its way_ to suppress such "redundant" string
> > literals?
>
> I wasn't suggesting it should; I was suggesting that Clang should be
> conservative and suppress printing the conditional when a message is present,
> not when they look to be redundant enough.
>
> > if a C++17-native program contains a string literal, we should maybe assume
> > it's on purpose.
>
> This is the exact scenario I was envisioning.
>
> It's a relatively weak preference, but I kind of prefer not displaying the
> conditional information in the presence of a message (at least in C++17 and
> above), especially as the conditional can be huge. I'm thinking of scenarios
> where the user does something like:
> ```
> static_assert(condition1 && condition2 && (condition3 || condition4), "Simple
> explanation");
> ```
> except that `condition` is replaced by `std::some_type_trait<Blah>` in
> various interesting ways.
>
> I'm thinking of scenarios where the user does something like:
>
> static_assert(condition1 && condition2 && (condition3 || condition4),
> "Simple explanation");
>
> except that condition is replaced by std::some_type_trait<Blah> in various
> interesting ways.
I think the risk is extremely high that "Simple explanation" will not be
actually useful to the person compiling the code, and they'll want to know
exactly what failed and why (which is the direction Clement's patches are
taking us). I also see the same trend happening with C++2a Concepts in
@saar.raz's fork: concept diagnostics are quite verbose compared to type-trait
diagnostics because it's useful to tell the user "hey, `Regular<T>` failed
because `Copyable<T>` failed because `MoveAssignable<T>` failed because..." and
eventually get all the way down to the _real_ problem, which ends up being
something like "`T` was deduced as `int&`". An additional "Simple explanation"
can be helpful, but is rarely as useful as the full story.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
