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

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

Quuxplusone added inline comments.


================
Comment at: test/SemaCXX/static-assert-cxx17.cpp:109
+template <class T, class U>
+void foo7() {
+  static_assert(X<typename T::T, U>());
----------------
courbet wrote:
> 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 ?
> 
> 
Here's what I wrote over on D55270.

>>! In D55270#1327704, @Quuxplusone wrote:
> @courbet: On the cpplang Slack, Peter Feichtinger mentioned that defaulted template arguments should perhaps be treated differently. Is there any way to suppress the `, std::allocator<int>` part of this diagnostic? https://godbolt.org/z/TM0UHc
> 
> Before your patches:
> ```
> <source>:11:5: error: static_assert failed due to requirement 'std::is_integral_v<typename S::t>'
>     static_assert(std::is_integral_v<typename T::t>);
>     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
> After your patches:
> ```
> <source>:11:5: error: static_assert failed due to requirement 'std::is_integral_v<std::vector<int, std::allocator<int> > >'
>     static_assert(std::is_integral_v<typename T::t>);
>     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 
> ```
> I don't think the new behavior is //worse//; but I don't think it's optimal, either.
> 
> I think Clang already has a feature to suppress printing these parameters; you would just have to figure out where it is and try to hook it into your thing. (And if you do, I'm going to ask for a test case where `T::t` is `std::pmr::vector<int>`!)

So does this patch (D55933) actually change the message printed by Peter's https://godbolt.org/z/TM0UHc ? (I think it does not.)
Does it change the message printed by https://godbolt.org/z/Q0AD70 ? (I think it does.)

I think most of your new test cases in `foo7` are redundant and don't really need to be there. Contrariwise, I would like to see one new test case isomorphic to https://godbolt.org/z/Q0AD70 , because that strikes me as a very realistic case that we want to protect against regressions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55933