On Fri, 22 Jan 2021, Patrick Palka wrote: > On Fri, 22 Jan 2021, Patrick Palka wrote: > > > On Thu, 21 Jan 2021, Jason Merrill wrote: > > > > > On 1/21/21 11:22 AM, Patrick Palka wrote: > > > > Here at parse time finish_qualified_id_expr adds an implicit 'this->' to > > > > the expression tmp::integral<T> (because it's type-dependent, and also > > > > current_class_ptr is set) within the trailing return type, and later > > > > during substitution we can't resolve the 'this' since > > > > tsubst_function_type does inject_this_parm only for non-static member > > > > functions which tmp::func is not. > > > > > > > > It seems the root of the problem is that we set current_class_ptr when > > > > parsing the signature of a static member function. Since explicit uses > > > > of 'this' are already not allowed in this context, we probably shouldn't > > > > be doing inject_this_parm either. > > > > > > Hmm, 'this' is defined in a static member function, it's just ill-formed > > > to > > > use it: > > > > > > 7.5.2/2: "... [this] shall not appear within the declaration of a static > > > member function (although its type and value category are defined within a > > > static member function as they are within a non-static member function). > > > [Note: This is because declaration matching does not occur until the > > > complete > > > declarator is known. — end note]" > > > > Ah, I see. > > > > > > > > Perhaps maybe_dummy_object needs to be smarter about recognizing static > > > context. Or perhaps there's no way to tell the difference between the > > > specified behavior above and the behavior with your patch, but: > > > > > > The note suggests that we need to test the case of an out-of-class > > > definition > > > of a static member function (template); we must inject_this_parm when > > > parsing > > > such a declaration, since we don't know it's static until we match it to > > > the > > > declaration in the class. I'm guessing that this would lead to the same > > > problem. > > > > Good point, we do get a signature mismatch error in this case, due to > > 'this->' appearing in the trailing return type out-of-class declaration > > but not in that of the in-class declaration, so the trailing return > > types don't match up. In light of this case, it doesn't seem possible > > for maybe_dummy_object to distinguish between a static and non-static > > context when parsing the trailing return type of the declaration. > > > > So perhaps we should mirror at substitution time what we do at parse > > time, and inject 'this' also when substituting into the function type of > > a static member function? That way, we'll resolve the use of 'this->' > > and then discard it the RHS of -> is a static member function, or > > complain that 'this' is not a constant expression if the RHS is a > > non-static member function. > > > > The following patch performs that, and seems to pass light testing. > > Full testing in progress. Revised commit message is still a WIP. > > How does this approach look?
Sorry for the spam, but I'd also like to propose a more conservative and targeted approach that basically undoes part of the r9-5972 patch which caused the regression. According to the email thread at https://gcc.gnu.org/pipermail/gcc-patches/2019-February/516421.html the hunk from r9-5972 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -2096,7 +2096,8 @@ finish_qualified_id_expr (tree qualifying_class, { /* See if any of the functions are non-static members. */ /* If so, the expression may be relative to 'this'. */ - if (!shared_member_p (expr) + if ((type_dependent_expression_p (expr) + || !shared_member_p (expr)) && current_class_ptr && DERIVED_FROM_P (qualifying_class, current_nonlambda_class_type ())) was added to avoid calling shared_member_p here when the overload set contains a dependent USING_DECL. But according to https://gcc.gnu.org/pipermail/gcc-patches/2018-December/513237.html if an overload set contains a dependent USING_DECL, then it'll be the first in the overload set. So we could partially revert the r9-5972 patch and do: diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c index c6b4c70dc0f..6b0d68ae4bf 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -2214,7 +2214,7 @@ finish_qualified_id_expr (tree qualifying_class, { /* See if any of the functions are non-static members. */ /* If so, the expression may be relative to 'this'. */ - if ((type_dependent_expression_p (expr) + if ((TREE_CODE (get_first_fn (expr)) == USING_DECL || !shared_member_p (expr)) && current_class_ptr && DERIVED_FROM_P (qualifying_class, The above pases 'make check-c++', and allows us to compile pr97399a.C successfully, but we'd still ICE on the invalid pr97399b.C. > > > > -- >8 -- > > > > Subject: [PATCH] c++: 'this' injection and static member functions [PR97399] > > > > gcc/cp/ChangeLog: > > > > PR c++/97399 > > * parser.c (cp_parser_init_declarator): If the storage class > > specifier is sc_static, pass true for static_p to > > cp_parser_declarator. > > (cp_parser_direct_declarator): Don't do inject_this_parm when > > the function is specified as a friend. > > * pt.c (tsubst_function_type): Also inject 'this' when > > substituting into the function type of a static member > > function. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/88548 > > PR c++/97399 > > * g++.dg/cpp0x/this2.C: New test. > > * g++.dg/gomp/this-1.C: Adjust expected error for use of 'this' > > in signature of static member function. > > * g++.dg/template/pr97399a.C: New test. > > * g++.dg/template/pr97399b.C: New test. > > --- > > gcc/cp/parser.c | 2 +- > > gcc/cp/pt.c | 13 +++++++++++-- > > gcc/testsuite/g++.dg/template/pr97399a.C | 23 +++++++++++++++++++++++ > > gcc/testsuite/g++.dg/template/pr97399b.C | 23 +++++++++++++++++++++++ > > 4 files changed, 58 insertions(+), 3 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/template/pr97399a.C > > create mode 100644 gcc/testsuite/g++.dg/template/pr97399b.C > > > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > > index 48437f23175..a0efa55d21e 100644 > > --- a/gcc/cp/parser.c > > +++ b/gcc/cp/parser.c > > @@ -22122,7 +22122,7 @@ cp_parser_direct_declarator (cp_parser* parser, > > > > tree save_ccp = current_class_ptr; > > tree save_ccr = current_class_ref; > > - if (memfn) > > + if (memfn && !friend_p) > > /* DR 1207: 'this' is in scope after the cv-quals. */ > > inject_this_parameter (current_class_type, cv_quals); > > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > > index ad855dbde72..d4763325e25 100644 > > --- a/gcc/cp/pt.c > > +++ b/gcc/cp/pt.c > > @@ -15072,8 +15072,17 @@ tsubst_function_type (tree t, > > > > tree save_ccp = current_class_ptr; > > tree save_ccr = current_class_ref; > > - tree this_type = (TREE_CODE (t) == METHOD_TYPE > > - ? TREE_TYPE (TREE_VALUE (arg_types)) : NULL_TREE); > > + tree this_type = NULL_TREE; > > + if (TREE_CODE (t) == METHOD_TYPE) > > + this_type = TREE_TYPE (TREE_VALUE (arg_types)); > > + else if (in_decl > > + && DECL_FUNCTION_MEMBER_P (in_decl) > > Oops, this line should instead be checking DECL_STATIC_FUNCTION_P. > Consider it changed. > > > + && t == TREE_TYPE (in_decl)) > > + /* Also inject 'this' when substituting into the function type > > + of a static member function, as we may have conservatively > > + added 'this->' to a dependent member function call in its > > + trailing return type which we might need to resolve. */ > > + this_type = DECL_CONTEXT (in_decl); > > bool do_inject = this_type && CLASS_TYPE_P (this_type); > > if (do_inject) > > { > > diff --git a/gcc/testsuite/g++.dg/template/pr97399a.C > > b/gcc/testsuite/g++.dg/template/pr97399a.C > > new file mode 100644 > > index 00000000000..4bb818908fd > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/template/pr97399a.C > > @@ -0,0 +1,23 @@ > > +// PR c++/97399 > > +// { dg-do compile { target c++11 } } > > + > > +template <bool> struct enable_if_t {}; > > + > > +struct tmp { > > + template <class> static constexpr bool is_integral(); > > + template <class T> static auto f() > > + -> enable_if_t<tmp::is_integral<T>()>; > > + template <class T> friend auto g(tmp, T) > > + -> enable_if_t<!tmp::is_integral<T>()>; > > +}; > > + > > +template <class> constexpr bool tmp::is_integral() { return true; } > > + > > +template <class T> auto tmp::f() > > + -> enable_if_t<tmp::is_integral<T>()> { return {}; } > > + > > +int main() > > +{ > > + tmp::f<int>(); > > + g(tmp{}, 0); > > +} > > diff --git a/gcc/testsuite/g++.dg/template/pr97399b.C > > b/gcc/testsuite/g++.dg/template/pr97399b.C > > new file mode 100644 > > index 00000000000..673ba6624e3 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/template/pr97399b.C > > @@ -0,0 +1,23 @@ > > +// PR c++/97399 > > +// { dg-do compile { target c++11 } } > > + > > +template <bool> struct enable_if_t {}; > > + > > +struct tmp { > > + template <class> constexpr bool is_integral(); // non-static > > + template <class T> static auto f() > > + -> enable_if_t<tmp::is_integral<T>()>; // { dg-message "in template > > argument" } > > + template <class T> friend auto g(tmp, T) > > + -> enable_if_t<!tmp::is_integral<T>()>; // { dg-error "without object" > > } > > +}; > > + > > +template <class> constexpr bool tmp::is_integral() { return true; } > > + > > +template <class T> auto tmp::f() // { dg-error "'this' is not a constant" } > > + -> enable_if_t<tmp::is_integral<T>()> { return {}; } > > + > > +int main() > > +{ > > + tmp::f<int>(); // { dg-error "no match" } > > + g(tmp{}, 0); // { dg-error "no match" } > > +} > > -- > > 2.30.0.155.g66e871b664 > >
