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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

Looks fine to me; please don't let //me// block this any further. :) Someone else, e.g. @aaron.ballman, should be the one to accept it, though.



================
Comment at: include/clang/AST/NestedNameSpecifier.h:220
+  void print(raw_ostream &OS, const PrintingPolicy &Policy,
+             bool ResolveTemplateArguments = false) const;
 
----------------
Peanut gallery says: Should `ResolveTemplateArguments` really be here, or should it be a property of the `PrintingPolicy` the same way e.g. `ConstantsAsWritten` is a property of the `PrintingPolicy`?  I don't know what a `PrintingPolicy` is, really, but I know that I hate defaulted boolean function parameters with a passion. ;)

Furthermore, I note that the doc-comment for `ConstantsAsWritten`, at line ~207 of include/clang/AST/PrettyPrinter.h, is nonsensical and maybe someone should give it some love. (That is totally not //your// problem, though.)


================
Comment at: lib/Sema/SemaTemplate.cpp:3071
+      printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+    }
+    return;
----------------
Checking my understanding: Am I correct that this code currently does not pretty-print

    static_assert(std::is_same<T, int>(), "");

(creating an object of the trait type and then using its constexpr `operator bool` to convert it to bool)? This is a rare idiom and doesn't need to be supported AFAIC.


================
Comment at: test/SemaCXX/static-assert.cpp:143
+void foo2() {
+  static_assert(::ns::NestedTemplates1<T, a>::NestedTemplates2::template NestedTemplates3<U>::value, "message");
+  // expected-error at -1{{static_assert failed due to requirement '::ns::NestedTemplates1<int, 3>::NestedTemplates2::NestedTemplates3<float>::value' "message"}}
----------------
Looking at this example, I thought of one more case to test.
```
    static_assert(std::is_same_v<T[sizeof(T)], int[4]>, "");
```
That is, I'd like to see a test that verifies whether `T[sizeof(T)]` pretty-prints as `float[sizeof(float)]`, or `float[4]`, or something else. (I don't really care what it prints as, as long as it's not completely crazy, and as long as it doesn't regress in future releases.)


================
Comment at: test/SemaCXX/static-assert.cpp:164
+  static_assert(std::is_const<typename T::iterator>::value, "message");
+  // expected-error at -1{{type 'int' cannot be used prior to '::' because it has no members}}
+}
----------------
I guess I meant more like
```
struct S {};
void bar() {
    foo(S{});
}
```
```
error: no member named 'iterator' in 'S'
    static_assert(std::is_same_v<typename T::iterator, int>);
                                          ~~~^
```
but as neither version goes through your codepath anyway, what you've got is OK.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54903