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@-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@-1{{due to requirement 'std::is_same<V<int, > > PMRA<int>>*, void>::value'}} > > // expected-error@-2{{due to requirement 'std::is_same<PMRV<int>*, > > void>::value'}} > > } > > template testy<PMRV<int>>(); > > ``` > > The `expected-error@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits