Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
858063002:
Clarify behavior of iterables and iterators with sync*. (Closed)
|
Created:
5 years, 11 months ago by gbracha Modified:
5 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionClarify behavior of iterables and iterators with sync*.
R=lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=43415
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Messages
Total messages: 10 (1 generated)
gbracha@google.com changed reviewers: + hausner@google.com, lrn@google.com
LGTM without IterableBase outside of commentary. https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.t... docs/language/dartLangSpec.tex:3508: If $f$ is marked \SYNC* (\ref{functions}), then a fresh instance $i$ implementing the built-in class \code{Iterable} is associated with the invocation and immediately returned. The behavior of the \code{Iterable} must be compatible with an extension of the \code{IterableBase} class in the \code{dart:core} library. No need to mention `IterableBase` in normative text (outside of the commentary) - it's just one implementation of the Iterable interface contract, and being compatible with that merely means implementing Iterable correctly. I assume "compatible with" means being extensionally equivalent, but IterableBase is just implementing Iterable, so it's sufficient to say that the sync*-Iterable correctly implements `Iterable`, which is where the interface is defined and documented. An "extension of the IterableBase class" can override all methods anyway, so being compatible with *an extension* is too vague to say anything. (Or is this because you prefer to specify behavior by an actual implementation instead of by an interface describing it?) https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.t... docs/language/dartLangSpec.tex:3520: One can derive more than one iterator from a given iterable. Note that operations on the iterable itself can create distinct iterators. An example would be \code{length}. It is conceivable that different iterators might yield sequences of different length. Unless the underlying \SYNC* function is a mathematically pure function, it is probably best not to create multiple iterators on the result of a \SYNC* function, either directly or indirectly. I wouldn't discourage creating multiple iterators. That is completely proper behavior for usage of an Iterable. I'd rather recommend that the function is written in such a way that this is not a problem, just as we do for Iterator implementations now. So maybe just say, replacing from "Unless ...": The same care needs to be taken when writing \code{sync*} functions as when writing an \code{Iterator} class. In particular, it should handle multiple simultaneous iterators gracefully. If the iterator depends on external state that might change, it should check that the state is still valid after every yield (and maybe throw a \code{ConcurrentModificationError} if it isn't). https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.t... docs/language/dartLangSpec.tex:3523: Each iterator runs with its own copies of all local variables; in particular, each iterator has the same initial arguments, even if their bindings are modified by the function. Good catch! There really is one invocation of the sync* function per active iterator, so we have to store the arguments in the Iterable until they are needed.
dbc https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.t... docs/language/dartLangSpec.tex:3523: Each iterator runs with its own copies of all local variables; in particular, each iterator has the same initial arguments, even if their bindings are modified by the function. What does "its own copies of all local variables" mean? If a parameter to the sync* function is a list, does the list have to be duplicated? I presume not, so two iterators can still communicate via their shared local values, despite the "cloning" of the initial values.
See if you can tolerate this formulation. I'm still relying on IterableBase. I'd rather take it out a sit is overspecification, but Itreable's dart doc doesn't cut it. https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.tex File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.t... docs/language/dartLangSpec.tex:3508: If $f$ is marked \SYNC* (\ref{functions}), then a fresh instance $i$ implementing the built-in class \code{Iterable} is associated with the invocation and immediately returned. The behavior of the \code{Iterable} must be compatible with an extension of the \code{IterableBase} class in the \code{dart:core} library. On 2015/01/21 10:19:21, Lasse Reichstein Nielsen wrote: > No need to mention `IterableBase` in normative text (outside of the commentary) > - it's just one implementation of the Iterable interface contract, and being > compatible with that merely means implementing Iterable correctly. > > I assume "compatible with" means being extensionally equivalent, but > IterableBase is just implementing Iterable, so it's sufficient to say that the > sync*-Iterable correctly implements `Iterable`, which is where the interface is > defined and documented. > > An "extension of the IterableBase class" can override all methods anyway, so > being compatible with *an extension* is too vague to say anything. > > (Or is this because you prefer to specify behavior by an actual implementation > instead of by an interface describing it?) I used IterableBase because I don't find the specification of Iterable to be very definitive. You are right about extension, I'll reword. https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.t... docs/language/dartLangSpec.tex:3520: One can derive more than one iterator from a given iterable. Note that operations on the iterable itself can create distinct iterators. An example would be \code{length}. It is conceivable that different iterators might yield sequences of different length. Unless the underlying \SYNC* function is a mathematically pure function, it is probably best not to create multiple iterators on the result of a \SYNC* function, either directly or indirectly. On 2015/01/21 10:19:21, Lasse Reichstein Nielsen wrote: > I wouldn't discourage creating multiple iterators. That is completely proper > behavior for usage of an Iterable. I'd rather recommend that the function is > written in such a way that this is not a problem, just as we do for Iterator > implementations now. > So maybe just say, replacing from "Unless ...": > > The same care needs to be taken when writing \code{sync*} functions as when > writing an \code{Iterator} class. In particular, it should handle multiple > simultaneous iterators gracefully. If the iterator depends on external state > that might change, it should check that the state is still valid after every > yield (and maybe throw a \code{ConcurrentModificationError} if it isn't). Done. https://codereview.chromium.org/858063002/diff/1/docs/language/dartLangSpec.t... docs/language/dartLangSpec.tex:3523: Each iterator runs with its own copies of all local variables; in particular, each iterator has the same initial arguments, even if their bindings are modified by the function. On 2015/01/21 17:33:03, hausner wrote: > What does "its own copies of all local variables" mean? If a parameter to the > sync* function is a list, does the list have to be duplicated? I presume not, so > two iterators can still communicate via their shared local values, despite the > "cloning" of the initial values. Shallow copy.
https://codereview.chromium.org/858063002/diff/40001/docs/language/dartLangSp... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/858063002/diff/40001/docs/language/dartLangSp... docs/language/dartLangSpec.tex:3508: If $f$ is marked \SYNC* (\ref{functions}), then a fresh instance $i$ implementing the built-in class \code{Iterable} is associated with the invocation and immediately returned. The behavior of the \code{Iterable} must be compatible with the \code{IterableBase} class in the \code{dart:core} library. Still doesn't *really* say anything (it's unspecified what it means to be "compatible" with a class). I think I know what it means - that the members other than "iterator" must behave in the same way depending on the iterator getter as the methods in IterableBase behaves depending on their iterator getter - but we can't expect all readers to read it that way. And it again just means that they must behave as specified by Iterable, because IterableBase gets its documentation from Iterable, and the only promises made by IterableBase are the documented behavior, not the actual implementation (although there shouldn't be any way to differ). If Iterable's documentation isn't sufficient, we should fix that, not bake an implementation helper class into the specification. I'll try adding some more precise documentation to Iterable.
On 2015/01/22 15:23:45, Lasse Reichstein Nielsen wrote: > https://codereview.chromium.org/858063002/diff/40001/docs/language/dartLangSp... > File docs/language/dartLangSpec.tex (right): > > https://codereview.chromium.org/858063002/diff/40001/docs/language/dartLangSp... > docs/language/dartLangSpec.tex:3508: If $f$ is marked \SYNC* (\ref{functions}), > then a fresh instance $i$ implementing the built-in class \code{Iterable} is > associated with the invocation and immediately returned. The behavior of the > \code{Iterable} must be compatible with the \code{IterableBase} class in the > \code{dart:core} library. > Still doesn't *really* say anything (it's unspecified what it means to be > "compatible" with a class). I think I know what it means - that the members > other than "iterator" must behave in the same way depending on the iterator > getter as the methods in IterableBase behaves depending on their iterator getter > - but we can't expect all readers to read it that way. > > And it again just means that they must behave as specified by Iterable, because > IterableBase gets its documentation from Iterable, and the only promises made by > IterableBase are the documented behavior, not the actual implementation > (although there shouldn't be any way to differ). > > If Iterable's documentation isn't sufficient, we should fix that, not bake an > implementation helper class into the specification. I'll try adding some more > precise documentation to Iterable. I agree. If you can tighten up the spec for Iterable so it really does dictate reasonable behavior, that would be best. I can hold off on this CL if necessary.
I've updated the spec in light of recent changes to the contract fro Itreable and Iterator. PTAL.
LGTM
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as r43415 (presubmit successful). |
