Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow unqualified name lookup for class members #2287
Allow unqualified name lookup for class members #2287
Changes from all commits
6af5930
3e429c7
bd947d8
8b0d50c
d9ca40c
2204d53
474cd62
edaf9c2
47e08ca
5480e06
a754ebb
9b5a0fe
9b73f28
64ef7a2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note about the open question to the proposal section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little funny to have the abstract be more concrete than the proposal section. Maybe swap the content of the two sections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abstract and the PR description are supposed to match. The text in the abstract is giving the amount of detail I believe to be correct here.
The text here is once sentence because of the paragraphs that follow it; that's my intent, to use sub-headers to explain what's only briefly noted in the abstract. It didn't seem long enough to be worth duplicating the abstract here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think the abstract and proposal text makes sense to me at this point? the abstract is a summary, and the subsections here seem to directly correspond so I think this is OK now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like us to consider the alternative of treating
me
as an ordinary parameter within method bodies, meaning that (modulo aliasing) it can be accessed only by explicitly naming it. In C++, I have often found it to be a readability obstacle that you can't easily find the places in a method body that access*this
. Treatingme
as an explicitly-declared parameter gives us a clear path to avoiding that problem, so if we're not going to take it, I think we should explain why.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this changes the use of
me
at all? My impression was that this only applies to unqualified lookup that doesn't use the instance at all (or*this
from C++), and only uses the type's scope.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea for the proposal to say something about this to avoid confusion. My interpretation is that given:
v = 1;
, unqualified lookup findsX.v
, so this is equivalent toX.v = 1;
, notme.v = 1;
X.v = 1;
, we are referring to an instance member without an instance, which is an error.me.v = 1;
, we perform instance binding and assign to the memberv
ofme
.me.(v) = 1;
, unqualified lookup findsX.v
, so this is equivalent tome.(X.v) = 1;
me.(X.v) = 1;
, we perform instance binding and assign to theX
memberv
ofme
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the missing part of zygoloid's example (see 981-985 in classes.md).
Note, my intent was just to fix out-of-date information and address what I thought would be a non-contentious change. I don't see
me
as either:I'm not familiar with the rationale behind disallowing
me.
references, nor could I find it considered anywhere.Requiring
me.
is consistent with C++ and seems more ergonomic to write (I don't think it's been a source of confusion in C++ code). Open design idea: implicit scoped function parameters #1974 also offers a way to make it work:me
could be an implicit scoped parameter.I'm not readily familiar with the argument for not supporting lookups with
me
. There may be refactoring reasons for disallowing it, but I would think those would be shared with unqualified name lookup in general.Because the issue hasn't been addressed previously and shares problems with unqualified name lookup on classes, I think it's fair to ask here. That said, I also don't feel comfortable resolving this, so I've added notes for an open question.
Is the open question a reasonable answer for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I overlooked the distinction between name lookup and instance binding, so I thought this was proposing to e.g. make
v = 1;
be valid and equivalent tome.v = 1
in @zygoloid's example. I've added a suggestion to the abstract that would have cleared up my confusion. With that change or something like it, I don't think we even need the open question (but it doesn't hurt to have it).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want some reference for this -- do you have an alternative reference that I can use in place of the open question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would stress either way, the open question seems fine.