Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[depr.func.adaptor.typedefs] Clarify that reference_wrapper does not define argument_type for non-nullary member function pointer types. #893

Merged
merged 1 commit into from Mar 20, 2017

Conversation

Eelis
Copy link
Contributor

@Eelis Eelis commented Aug 6, 2016

No description provided.

@zygoloid zygoloid added the lwg Issue must be reviewed by LWG. label Nov 12, 2016
@@ -1468,8 +1468,7 @@
only if the type \tcode{T} is any of the following:
\begin{itemize}
\item a function type or a pointer to function type taking one argument of type \tcode{T1}
% FIXME: Should this be R T0::f() ?
\item a pointer to member function \tcode{R T0::f} \cv{} (where \cv{} represents the member function's cv-qualifiers); the type \tcode{T1} is \cv{} \tcode{T0*}
\item a pointer to member function \tcode{R T0::f()} \cv{} (where \cv{} represents the member function's cv-qualifiers); the type \tcode{T1} is \cv{} \tcode{T0*}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wording still doesn't make any sense. Did this perhaps mean R T0::*f() cv? Or is this supposed to be a declaration of a member function? Perhaps LWG can give us some guidance about what was intended here.

Copy link
Contributor Author

@Eelis Eelis Nov 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense with the fix. As an example of what the item is talking about, consider the member function:

size_t string::size() const

(R=size_t, T0=string, f=size, cv=const)

The item says that if T (reference_wrapper's parameter) is a pointer to this thing, then T1 (the resulting argument_type) is cv T0*, which is const string*.

So it is saying that reference_wrapper<size_t(string::*)()const>::argument_type is const string*, which is also what I get with libstdc++ and libc++.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I agree it's sloppy and confusing phrasing though.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is talking about a type T. There's no way a type can be "a pointer to member function size_t string::size() const" -- that's not a property of a type.

My concerns here are twofold:

  1. This may be a normative change, depending on whether an argument_type was also supposed to be defined for a pointer to member function with parameters or not. (With your change we seem to say only the no-parameter case is covered, whereas without it, this appears to apply regardless of whether the function has parameters.)
  2. This change still leaves the wording meaningless.

I think we do want to make a change in this direction (and I suspect the right change would be to use R T0::*f()cv), but I'd like LWG to look at this and decide what exactly to do.

Copy link
Member

@jwakely jwakely Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is supposed to be R T0::f() cv, and should be read as:

a pointer, to a member function like R T0::f() cv

i.e.

a pointer to member function that points to a member function such as R T0::f() cv

It certainly would be clearer to talk about the type of the pointer to member function, not show a declaration of a member function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. Eelis's suggestion is not a normative change, the intention, and I believe what all implementations do, is to only define argument_type when it's a pointer to member function for a member function taking no parameters.

For a member function taking exactly one parameter first_argument_type and second_argument_type are defined.

For member functions taking two or more parameters none of those typedefs is defined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tr1::reference_wrapper says:

— a pointer to member function type with cv-qualifier cv and no arguments; the type T1 is cv T* and R is the return type of the pointer to member function

and then for the R T0::f(T2) case:

— a pointer to member function with cv-qualifier cv and taking one argument of type T2; the type T1 is cv T* and R is the return type of the pointer to member function

So at some point we rephrased that and mangled it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was changed by http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#521 and it was meant to have the () but the editor didn't apply the DR correctly.

@tkoeppe
Copy link
Contributor

tkoeppe commented Mar 20, 2017

@zygoloid: What's the status of this pull request? Did anyone ever ask LWG for input?

@mclow: Could you perhaps advise on whether a) this is something worth revising, and b) whether this is editorial?

…define argument_type for non-nullary member function pointer types.
@Eelis
Copy link
Contributor Author

Eelis commented Mar 20, 2017

Rebased

@jwakely
Copy link
Member

jwakely commented Mar 20, 2017

As I said in the now-collapsed comments on the original commit, the proposed change simply fixes the missing () from the resolution of LWG DR 521. Which definitely qualifies as editorial.

I can't speak for @mclow but my recommendation would be to merge this and not spend time trying to improve the wording for these deprecated typedefs.

@tkoeppe
Copy link
Contributor

tkoeppe commented Mar 20, 2017

@jwakely: OK, indeed. Merge that noise.

@mclow
Copy link
Contributor

mclow commented Mar 20, 2017

OK, indeed. Merge that noise.
Works for me.

@zygoloid zygoloid removed the lwg Issue must be reviewed by LWG. label Mar 20, 2017
@zygoloid zygoloid merged commit 660d97e into cplusplus:master Mar 20, 2017
@Eelis Eelis deleted the refwrap branch March 20, 2017 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants