[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: test/PCH/cxx-static_assert.cpp:17
 
-// expected-error at 12 {{static_assert failed "N is not 2!"}}
+// expected-error at 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:
> 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.



Repository:
  rC Clang

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

https://reviews.llvm.org/D55270