courbet 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>());
----------------
Quuxplusone wrote:
> 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.
> So does this patch (D55933) actually change the message printed by Peter's
> https://godbolt.org/z/TM0UHc ? (I think it does not.)
This is correct, it does not.
> Does it change the message printed by https://godbolt.org/z/Q0AD70 ? (I think
> it does.)
It does.
> I think most of your new test cases in foo7 are redundant and don't really
> need to be there.
Sound good, but I've still kept a few cases for coverage.
> 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.
Done.
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
