[PATCH] D55933: [Sema] Do not print default template parameters.

[Clement Courbet via Phabricator via cfe-commits]

courbet marked 6 inline comments as done.
courbet added inline comments.


================
Comment at: lib/AST/TypePrinter.cpp:173
+  // information about whether default template parameters were actually
+  // written.
+  switch (QT.getTypePtr()->getTypeClass()) {
----------------
Quuxplusone wrote:
> FWIW, I don't understand this comment's terminology. "Whether default template parameters were actually written": should that say something like "whether a template parameter came from a default argument"?
> And why are the `Pointer`, `VariableArray`, etc. cases here? What goes wrong if you remove them?
I've expanded the comments.


================
Comment at: test/SemaCXX/static-assert-cxx17.cpp:99
   static_assert((const X<typename T::T>[]){} == nullptr);
-  // expected-error at -1{{static_assert failed due to requirement '(X<int> const[0]){} == nullptr'}}
+  // expected-error at -1{{static_assert failed due to requirement '(const X<int> [0]){} == nullptr'}}
   static_assert(sizeof(X<decltype(X<typename T::T>().X<typename T::T>::~X())>) == 0);
----------------
Quuxplusone wrote:
> (1) This cosmetic change is produced by the `case IncompleteArray:` that I questioned above, right? I'm surprised that the pretty-printed type changes from "east const" to "west const." But that's fine.
> 
> (2) Sorry I didn't notice before: surely where the message currently says `[0]` it should say `[]` instead! That's an existing bug in the pretty-printer, going back super far: https://godbolt.org/z/y9KzEq  So I guess it's not your responsibility to fix. But I hope there's a bug open about it somewhere?
> (1) This cosmetic change is produced by the case IncompleteArray: that I questioned above, right? I'm surprised that the pretty-printed type changes from "east const" to "west const." But that's fine.

Indeed. There's some logic in the printer to see where the const should go. 

> (2) Sorry I didn't notice before: surely where the message currently says [0] it should say [] instead! That's an existing bug in the pretty-printer, going back super far: https://godbolt.org/z/y9KzEq So I guess it's not your responsibility to fix. But I hope there's a bug open about it somewhere?

Is it actually a bug ? In the godbolt example I actually thing putting the zero there actually clarifies the semantics of the expression for the reader, so I would'nt say it hurts.



================
Comment at: test/SemaCXX/static-assert-cxx17.cpp:103
+  static_assert(constexpr_return_false<typename T::T>());
+  // expected-error at -1{{static_assert failed due to requirement 'constexpr_return_false<int>()'}}
 }
----------------
Quuxplusone wrote:
> Please keep the old test case 
> 
>     static_assert(constexpr_return_false<typename T::T, typename T::U>());
>     // expected-error at -1{{static_assert failed due to requirement 'constexpr_return_false<int, float>()'}}
> 
> and then //add// this new test case. My understanding is that the old test case should still pass, even after your change.
The old one is in foo7().


================
Comment at: test/SemaCXX/static-assert-cxx17.cpp:109
+template <class T, class U>
+void foo7() {
+  static_assert(X<typename T::T, U>());
----------------
Quuxplusone wrote:
> None of these new test cases actually involve the default template argument.
> 
> I'd like to see two test cases explicitly mimicking `vector`. (Warning: I didn't run this code!)
> ```
> template<class> struct A {};
> template<class, class = A<T>> struct V {};
> template<class C> void testx() {
>     static_assert(std::is_same<C*, void>::value, "");
>     // expected-error at -1{{due to requirement 'std::is_same<V<int>*, void>::value'}}
> }
> template testx<V<int>>();
> template<class> struct PMRA {};
> template<class T> using PMRV = V<T, PMRA<T>>;
> template<class C> void test() {
>     static_assert(std::is_same<C*, void>::value, "");
>     // expected-error at -1{{due to requirement 'std::is_same<V<int, PMRA<int>>*, void>::value'}}
>     // expected-error at -2{{due to requirement 'std::is_same<PMRV<int>*, void>::value'}}
> }
> template testy<PMRV<int>>();
> ```
> The `expected-error at -2` above is my fantasy world. I don't think it's possible to achieve in real life. :)
> None of these new test cases actually involve the default template argument.

This one is to check that we actually do print when specified explicitly. foo6 above tests  the default template arguments (notice the change from `template <class T> struct X to `template <class T, class U = double> struct X` above). I've renamed `foo6` and `foo7` to make that clear.

Before this change, static_asserts in `foo6` and `foo7` printed the same thing. Now they don't.

> I'd like to see two test cases explicitly mimicking vector.

OK, I think I misunderstood what you wanted here. I don't think what you want is actually doable, because by the time you're in `test()`, C is just a type without any way of knowing whether the user explicitly provided the template parameter or relied on the default.

What we could do though is **always** erase template parameters that are the same as default ones. But that would mean printing `due to requirement 'std::is_same<V<int>*, void>::value'` even when the user wrote `template testx<V<int, A<int>>>();`.
WDYT ?




================
Comment at: test/SemaCXX/static-assert.cpp:130
 static_assert(std::is_same<decltype(std::is_const<const ExampleTypes::T>()), int>::value, "message");
-// expected-error at -1{{static_assert failed due to requirement 'std::is_same<std::is_const<const int>, int>::value' "message"}}
+// expected-error at -1{{static_assert failed due to requirement 'std::is_same<is_const<const int>, int>::value' "message"}}
 static_assert(std::is_const<decltype(ExampleTypes::T(3))>::value, "message");
----------------
Quuxplusone wrote:
> Why did this output change?
This changed in the parent diff D55933.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55933