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

[Aaron Ballman via Phabricator via cfe-commits]

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 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:
> 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