aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
================
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}}
----------------
Quuxplusone wrote:
> 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.
>
Okay, I find that to be a compelling argument, thank you @Quuxplusone!
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
