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

[Clement Courbet via Phabricator via cfe-commits]

courbet marked an inline comment as done.
courbet added a comment.

Thanks for the comments.



================
Comment at: lib/Sema/SemaTemplate.cpp:3070
+// and returns true.
+static bool prettyPrintTypeTrait(const NestedNameSpecifier *const NNS,
+                                 llvm::raw_string_ostream &OS,
----------------
aaron.ballman wrote:
> No need for the pointer itself to be `const` qualified -- drop the top-level `const` qualifier (here and elsewhere).
constness prevents me from accidentally reassigning the variable. But I won't fight over it :)


================
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)) {
----------------
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.


================
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"}}
----------------
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.



Repository:
  rC Clang

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

https://reviews.llvm.org/D54903