[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaTemplate.cpp:3076
+    return false;
+  const RecordDecl * Record = NNS->getAsRecordDecl();
+  // In namespace "::std".
----------------
Formatting is incorrect here; you should run the patch through clang-format.

Can `getAsRecordDecl()` return null even when looking for a type out of a NNS? If so, you should assert/test for that.


================
Comment at: lib/Sema/SemaTemplate.cpp:3115
+    // This might be `std::some_type_trait<U,V>::value`.
+    if (Var && Var->isStaticDataMember() && Var->getName() == "value" &&
+        prettyPrintTypeTrait(DR->getQualifier(), OS, PrintPolicy)) {
----------------
courbet wrote:
> aaron.ballman wrote:
> > You can also check `Var->isInStdNamespace()` here to simplify the logic above.
> Thanks for the pointer ! I was looking for something like this :)
> I still have to check this on the qualifier and not the variable though, but that does make the logic a lot simpler.
Ah, good point!


================
Comment at: test/SemaCXX/static-assert.cpp:111
+static_assert(std::is_same<ExampleTypes::T, ExampleTypes::U>::value, "message"); // expected-error{{static_assert failed due to requirement 'std::is_same<int, float>::value' "message"}}
+static_assert(std::is_const<ExampleTypes::T>::value, "message");                 // expected-error{{static_assert failed due to requirement 'std::is_const<int>::value' "message"}}
----------------
courbet wrote:
> Quuxplusone wrote:
> > I would like to see some more realistic test cases. I suggest this test case for example:
> > ```
> > struct BI_tag {};
> > struct RAI_tag : BI_tag {};
> > struct MyIterator {
> >     using tag = BI_tag;
> > };
> > struct MyContainer {
> >     using iterator = MyIterator;
> > };
> > template<class Container>
> > void foo() {
> >     static_assert(std::is_base_of_v<RAI_tag, typename Container::iterator::tag>);
> > }
> > ```
> > This is an example where as a programmer I would not want to see //only// `failed due to requirement std::is_base_of_v<RAI_tag, BI_tag>` — that doesn't help me solve the issue. OTOH, since every diagnostic includes a cursor to the exact text of the `static_assert` already, I think it's fair to say that the current diagnostic message is redundant, and therefore it's okay to replace it (as you propose to do) with something that is not redundant.
> > I think it's fair to say that the current diagnostic message is redundant, and therefore it's okay to replace it (as you propose to do) with something that is not redundant.
> 
> Yes, the proposal here might not be the *best* possible diagnostic for all cases, but it's already a huge improvement on the existing one, and solves a significant proportion of use cases.
> 
> Here, the programmer will see:
> ```
> test.cc:13:5: error: static_assert failed due to requirement 'std::is_base_of<RAI_tag, BI_tag>::value'
>     static_assert(std::is_base_of<RAI_tag, typename Container::iterator::tag>::value);
>     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
> which I think is a reasonable help for debugging.
> 
@Quuxplusone, do you have recommendations for what you'd prefer to see instead?

FWIW, I think this is a good incremental improvement. If there's more information we could display easily as part of this patch, we should consider it, but I'm also fine with saying this is progress.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54903