This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Do not print default template parameters.
Needs ReviewPublic

Authored by courbet on Dec 20 2018, 7:40 AM.

Details

Summary

As a followup to D55270.

Diff Detail

Event Timeline

courbet created this revision.Dec 20 2018, 7:40 AM
lib/AST/TypePrinter.cpp
173

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?

test/SemaCXX/static-assert-cxx17.cpp
104

(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?

121

Please keep the old test case

static_assert(constexpr_return_false<typename T::T, typename T::U>());
// expected-error@-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.

125

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. :)

test/SemaCXX/static-assert.cpp
130

Why did this output change?

courbet marked 6 inline comments as done.Dec 21 2018, 1:43 AM
courbet added inline comments.
lib/AST/TypePrinter.cpp
173

I've expanded the comments.

test/SemaCXX/static-assert-cxx17.cpp
104

(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.

121

The old one is in foo7().

125

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 ?

test/SemaCXX/static-assert.cpp
130

This changed in the parent diff D55933.

test/SemaCXX/static-assert-cxx17.cpp
125

Here's what I wrote over on D55270.

@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.

courbet updated this revision to Diff 180035.Jan 3 2019, 4:11 AM
courbet marked 2 inline comments as done.

Add more tests as suggested in the comments.

test/SemaCXX/static-assert-cxx17.cpp
125

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.

Quuxplusone added inline comments.Jan 3 2019, 9:05 AM
lib/AST/TypePrinter.cpp
179

Nit: there's an extra ` on this line.

I think I vaguely understand the purpose of this switch now. It feels ugly, but I have no concrete suggestion for improvement. Do you not need a case here to delay canonicalization of X<Some::Class>& also?

courbet updated this revision to Diff 180220.Jan 4 2019, 4:12 AM
courbet marked 2 inline comments as done.

Handle {L,R}Value references.

lib/AST/TypePrinter.cpp
179

Indeed. Fixed + added tests.

lib/AST/TypePrinter.cpp
179

And void(*)(X<Some::Class>)? And possibly int[X<Some::Class>::value], but I think that might be what Type::DependentSizedArray is doing in there. And void (^)(X<Some::Class>) in Objective-C++.

Surely there should be a list somewhere of all the "compound types" that need to go here. And then this starts feeling a lot like the visitor pattern. I still don't know how it _should_ look, but this switch is feeling ickier and ickier.