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@-1{{static_assert failed due to requirement '(X<int>
const[0]){} == nullptr'}}
+ // expected-error@-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@-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@-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@-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 ?
================
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@-1{{static_assert failed due to requirement
'std::is_same<std::is_const<const int>, int>::value' "message"}}
+// expected-error@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
