Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2060773002:
Various fixes to wording and typos. (Closed)
|
Created:
4 years, 6 months ago by Lasse Reichstein Nielsen Modified:
4 years, 6 months ago CC:
reviews_dartlang.org, floitsch Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionVarious fixes to wording and typos.
R=eernst@google.com
Committed: https://github.com/dart-lang/sdk/commit/65421fb2bf64efb7f3dcccb2de4ba349c9725dad
Patch Set 1 #Patch Set 2 : Remove trailing whitespace. #
Total comments: 25
Patch Set 3 : Address comment #
Total comments: 1
Messages
Total messages: 9 (3 generated)
lrn@google.com changed reviewers: + eernst@google.com
Inserted a number of comments, many of them to indicate that I think a seemingly nontrivial changes is actually trivial: It was just diff getting confused about the removal of some trailing white space. But there are still a few non-trivial comments: One change introduces a `p` that (seemingly) has no definition, and another one enables mixins to have additional members (factory constructors), and I think we'd need to go over them an extra time. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:11: {\large Version 1.14}} So there will never be a version 1.12 nor 1.13? (Or they were never committed to the repo, but they were sent out somewhere?) https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:327: {\em Run-time errors} are exceptions raised during execution. Whenever we say that an exception $ex$ is {\em raised} or {\em thrown}, we mean that a throw expression (\ref{throw}) of the form: \code{\THROW{} $ex$;} was implicitly evaluated or that a rethrow statement (\ref{rethrow}) of the form \code{\RETHROW} was executed. When we say that {\em a} $C$ {\em is thrown}, where $C$ is a class, we mean that an instance of class $C$ is thrown. When we say that a stream raises an exception, we mean that an exception occurred while computing the value(s) of the stream. As far as I can see we have exactly one occurrence of "<a stream> raises an exception" in the spec, and now it's under control. OK! https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2016: It is a compile-time error if a declared or derived mixin explicitly declares a constructor which is not a factory constructor. We should make sure we understand the consequences of this change. Was it omitted by accident and all tools do allow these factory constructors, or is it a "benign" extension, etc? https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3525: If $f$ is marked \ASYNC{} (\ref{functions}), then a fresh instance (\ref{generativeConstructors}) $o$ implementing the built-in class \code{Future} is associated with the invocation and immediately returned to the caller. The body of $f$ is scheduled for execution at some future time. The future $o$ will complete when $f$ terminates. The value used to complete $o$ is the current return value (\ref{return}), if it is defined, and the current exception (\ref{throw}) otherwise. Not obvious why the diff algorithm detects a change here: I cannot see that anything has changed. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4164: It is a compile-time error if $e$ is a prefix object (\ref{imports}) and $m$ refers to a type accessible via $p$ or to a member of class \cd{Object}. I cannot see `p` being introduced anywhere prior to this point in `\subsubsection{Super Getter Access and Method Closurization}`, so we would need to recover the intention behind this change and adjust it such that it is well-defined. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4711: A logical boolean expression $b$ of the form $e_1 \&\& e_2$ shows that a variable $v$ has type Again, I consider this to be an artifact of diff confusion (due to removed trailing spaces, presumably), I cannot see that anything has changed. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:5797: \LMHash{} No actual changes spotted here. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:6216: \item If the body of $f$ is marked \ASYNC{} (\ref{functions}) it is a dynamic type error if $o$ is not \NULL{} (\ref{null}) and \code{Future$<$flatten(S)$>$} is not a subtype of the actual return type (\ref{actualTypeOfADeclaration}) of $f$. OK. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:6325: If $s_E$ is an asynchronous for loop (\ref{asynchronousFor-in}), its associated stream subscription is canceled. Furthermore, let $a_k$ be the set of asynchronous for loops and yield-each statements (\ref{yieldEach}) enclosing $s_b$ that are enclosed in $s_E , 1 \le k \le m$, where $a_k$ is enclosed in $a_{k+1}$. The stream subscriptions associated with $a_j$ are canceled, $1 \le j \le m$, innermost first, so that $a_j$ is canceled before $a_{j+1}$. No actual changes detected here. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7199: This is the second guillemet, did we check whether they are an endangered species? After all, it's red in Rietveld.. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7338: \item For all $y_i \in \{y_1, \ldots, y_m\}, y_i = x_j \Rightarrow T_{x_j} \Longleftrightarrow S_{y_i}$ That was a thinko. Since $y_i = x_j$ it might be better for readability to introduce the shared name and use that as the index on both $T$ and $S$, but it is still OK as it is. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7581: where $L_i$ Again, I cannot spot any actual changes here, diff must have been confused by the trailing space in line 7560. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7655: {\em Documentation comments} are comments that begin with the tokens \code{///} or \code{/**}. Documentation comments are intended to be processed by a tool that produces human readable documentation. Again no actual changes, but we might as well delete the spurious period at the end of line 7649.
https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:11: {\large Version 1.14}} No idea. I also don't know what the number means, so I kept Gilad's numbering. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2016: It is a compile-time error if a declared or derived mixin explicitly declares a constructor which is not a factory constructor. I don't know whether it was omitted by accident or later-regretted design, but all tools seem to support factory constructors on mixins. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3525: If $f$ is marked \ASYNC{} (\ref{functions}), then a fresh instance (\ref{generativeConstructors}) $o$ implementing the built-in class \code{Future} is associated with the invocation and immediately returned to the caller. The body of $f$ is scheduled for execution at some future time. The future $o$ will complete when $f$ terminates. The value used to complete $o$ is the current return value (\ref{return}), if it is defined, and the current exception (\ref{throw}) otherwise. Trailing space removed. My editor does that automatically, so I want to get rid of them once and for all. Once and for all! If you check the previous revision of the CL, you'll see just the meaningful changes. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4164: It is a compile-time error if $e$ is a prefix object (\ref{imports}) and $m$ refers to a type accessible via $p$ or to a member of class \cd{Object}. Seems to me that $p$ should be the prefix object's name. Then the sentence makes sense. That is: ... if $e$ is a prefix object, $p$, (\ref{imports}) ... https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4711: A logical boolean expression $b$ of the form $e_1 \&\& e_2$ shows that a variable $v$ has type It is indeed a trailing space again. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7199: I think it represents a tab character, but I don't know for sure. I never use tab characters for anything.
LGTM https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:11: {\large Version 1.14}} On 2016/06/14 12:00:01, Lasse Reichstein Nielsen wrote: > No idea. I also don't know what the number means, so I kept Gilad's numbering. Acknowledged. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2016: It is a compile-time error if a declared or derived mixin explicitly declares a constructor which is not a factory constructor. On 2016/06/14 12:00:01, Lasse Reichstein Nielsen wrote: > I don't know whether it was omitted by accident or later-regretted design, but > all tools seem to support factory constructors on mixins. OK, let's keep it! https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:3525: If $f$ is marked \ASYNC{} (\ref{functions}), then a fresh instance (\ref{generativeConstructors}) $o$ implementing the built-in class \code{Future} is associated with the invocation and immediately returned to the caller. The body of $f$ is scheduled for execution at some future time. The future $o$ will complete when $f$ terminates. The value used to complete $o$ is the current return value (\ref{return}), if it is defined, and the current exception (\ref{throw}) otherwise. On 2016/06/14 12:00:01, Lasse Reichstein Nielsen wrote: > Trailing space removed. My editor does that automatically, so I want to get rid > of them once and for all. Once and for all! > > If you check the previous revision of the CL, you'll see just the meaningful > changes. Acknowledged. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4164: It is a compile-time error if $e$ is a prefix object (\ref{imports}) and $m$ refers to a type accessible via $p$ or to a member of class \cd{Object}. On 2016/06/14 12:00:01, Lasse Reichstein Nielsen wrote: > Seems to me that $p$ should be the prefix object's name. Then the sentence makes > sense. > That is: > ... if $e$ is a prefix object, $p$, (\ref{imports}) ... Yes, that was also the kind of thing I expected to find somewhere. The reference to members of class Object is a bit surprising, but assuming that the spec in general keeps that point of view in mind ("a prefix object is an object") we should keep it, for now. So let's add the $p$. I think the typical way to do this is to say 'the expression $e$ is evaluated to an object $o$', with subsequent references to $o$. A similar thing should happen to $e$ here, but evaluation may be known to be "const-like" in the case where the result is a prefix object, which makes it natural to omit "is evaluated to". But then we might still use 'if $e$ evaluates to a prefix object $p$', simply because even trivial evaluation fits into the context here and there is no reason to introduce a magic "is" meaning "evaluate-except-that-there-aint-really-nothing-going-on". https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:4711: A logical boolean expression $b$ of the form $e_1 \&\& e_2$ shows that a variable $v$ has type On 2016/06/14 12:00:01, Lasse Reichstein Nielsen wrote: > It is indeed a trailing space again. Acknowledged. https://codereview.chromium.org/2060773002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7199: On 2016/06/14 12:00:01, Lasse Reichstein Nielsen wrote: > I think it represents a tab character, but I don't know for sure. I never use > tab characters for anything. Acknowledged.
Description was changed from ========== Various fixes to wording and typos. ========== to ========== Various fixes to wording and typos. R=eernst@google.com Committed: https://github.com/dart-lang/sdk/commit/65421fb2bf64efb7f3dcccb2de4ba349c9725dad ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 65421fb2bf64efb7f3dcccb2de4ba349c9725dad (presubmit successful).
Message was sent while issue was closed.
floitsch@google.com changed reviewers: + floitsch@google.com
Message was sent while issue was closed.
DBC. Is there a changelog file somewhere? https://codereview.chromium.org/2060773002/diff/40001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2060773002/diff/40001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2491: The answer is that there are contexts where $e_1$ is a variable. In particular, constant constructor initializers such as This sentence feels incomplete. |
