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
ranged-based
for
for user-defined types #1885ranged-based
for
for user-defined types #1885Changes from 12 commits
c7b8ccf
a355466
ef76d36
83b208c
217ee4e
02ee7f8
0471f8d
cf71002
91d9709
c90d7ba
c9bbad7
0152f41
00ef294
bfcb08c
4fe274f
936bedc
b435c6f
76f13e4
0d593e1
dd6e4b1
48c95f7
bf7330b
51aabf4
6f2dd82
40716c7
c714989
5c653a7
c897aad
6d57ec8
12e0ff9
471a7af
3efeb38
0108dba
0c45a70
06bb190
c8d8bcf
845e0aa
f90fbfd
31f9361
f5eea57
59637d2
63f927c
cc86c60
80a74cd
4fad5ef
ff102e4
caa8d86
fe33c52
7b81a95
0e545d8
8e1af85
4277d37
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.
Two things:
begin()
andend()
is what has to support these operators.for
loop gets rewritten to includefor ( ; __begin != __end; ++__begin)
.We may need to be careful about closely matching C++ so we can be as compatible as we can manage, as long as it doesn't impact the usability for ordinary users. In C++, my understanding is as long as the rewrite type checks, the ranged-based for loop is legal, like a template. This means we may need to support
begin()
andend()
returning different types, as long as!=
is defined on those two types.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.
Absolutely yes, clarified.
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 this will still work if
ElementType
is move-only (but see the comment below), it just means thatIterateIterator
will be move-only in those cases. That means it won't be a valid iterator according to the standard library, but it should still work with range-basedfor
loops.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 just looked back at the code:
Won't
Next()
requireElementType
to be copyable nonetheless, as we are effectively returning a copy of an element, and not moving anything from the container? Even if we avoid an optional-like wrapper, or just move from it, theElementType
still has to be copied at some point (likely withingNext()
).Maybe I forgot something between now and then.
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.
In recent versions of C++, if
Next
returns anElementType
rather than a reference, the code you quote won't actually copy or move the return value; it will usev
itself as the storage for the returned temporary. Now, you're right that in this context, the body ofNext
will very likely need to make a copy, butNext
is written in pure Carbon, so that problem isn't caused by caching in the interoperability layer, and in fact it's not caused by the interop layer at all -- the same problem exists in pure Carbon.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.
Agree that this is by design, and not caused by the interop. To expand, I think we could say "move-only" in the C++ domain, most that would be ignoring a very likely "copiable" requirement on Carbon side /
Next
. So all-in-all, "copyable" seem more correct, and the upper bound of reqs, vs the lower bound.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 problem is that this doc doesn't explicitly talk about copyability on the Carbon side, so by introducing it here as a new topic that's specifically tied to caching in the interop layer, it really makes it sound like native Carbon will not require copyability, but C++ interop will.
I'd suggest revising the "Large elements and Optional" section to explicitly talk about the fact that this design may force
Next
to make a copy, and hence requireElementType
to be copyable. Then down here you can say that the caching requiresElementType
to be movable, and note that this won't matter in practice if Carbon requires it to be copyable anyway.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.
What is
Iterable<...>
here? It looks likeIterateIterator
expects to be parameterized by a container type. IsIterable
some sort of adapter to define theCursorType
andElementType
members? I don't see it defined anywhere else in this file.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.
That's a typo, and should be
Iterate<C, T>
, which is the C++ equivalent to Carbon's type. Fixed.