Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2665613003:
Fix specification of multiline strings. (Closed)
|
Created:
3 years, 10 months ago by Lasse Reichstein Nielsen Modified:
3 years, 10 months ago CC:
reviews_dartlang.org, Johnni Winther Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix specification of multiline strings.
Now say that NEWLINE is one of CR, LF or CR+LF, and that all of these
contribute a single LF to the string value.
Fixes issue #14073
BUG= http://dartbug.com/14073
R=eernst@google.com, johnniwinther@google.com
Committed: https://github.com/dart-lang/sdk/commit/ef31637857d03adc302c821906b7772ccf86c29d
Patch Set 1 #Patch Set 2 : Add comment. #
Total comments: 15
Patch Set 3 : Address comments #
Total comments: 2
Messages
Total messages: 10 (3 generated)
lrn@google.com changed reviewers: + eernst@google.com
johnniwinther@google.com changed reviewers: + johnniwinther@google.com
lgtm
LGTM. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2852: However, the use the concatenation operation is still discouraged for efficiency reasons. Instead, the recommended Dart idiom is to use string interpolation. 'the use [of?] the' https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2877: {\bf multilineString:}`{\escapegrammar \texttt{"""}}' stringContentTDQ* `{\escapegrammar \texttt{"""}}'; Might as well fix the triple space issue here. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2878: `{\escapegrammar \code{'}\code{'}\code{'}}' stringContentTSQ* `{\escapegrammar \code{'}\code{'}\code{'}}'; Having touched this, couldn't we simplify it to `\code{'''}` rather than repeating `\code` three times? Does `''` somehow encode `"`? https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2879: `r' `{\escapegrammar \texttt{"""}}' (\~{} `{\escapegrammar \texttt{"""}}')* `{\escapegrammar \texttt{"""}}'; Double space again. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2880: `r' `{\escapegrammar \code{'}\code{'}\code{'}}' (\~{} `{\escapegrammar \code{'}\code{'}\code{'}}')* `{\escapegrammar \code{'}\code{'}\code{'}}' Same potential simplification of 3*code. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2906: The idea is to ignore whitespace, where whitespace is defined as tabs, spaces and the final newline. These can be represented directly, but since for most characters prefixing by backslash is an identity in a non-raw string, we allow those forms as well. This might be better: 'the final newline' --> 'the newline at the end of the first line'. With this, we avoid readers thinking that this is about the multi-line string as a whole (so there may be several newline characters and the 'final' one could be in line 7). This is based on the assumption that the topic is the first line of the multi-line string. If we don't agree on that assumption then some other clarification is needed. ;-) https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2932: Multiline strings can contain newlines. A carriage return character followed by a newline character is considered a single newline. Newlines in multiline string literals contribute a single newline character to the string value. We need to make it obvious that these 'newlines' are "raw", i.e., that they are not escape sequences in the string. Maybe it would be good for the general readability to extend it a bit: `can contain raw line breaking character sequences (not escape sequences).` This avoids the use of the word 'newlines', which means that we can include `\r` and `\r\n` without conflicting with the identity '\n' == '\x0A'. We can then continue with `The following line breaking character sequences are recognized: \n, \r, \r\n. Each of them contributes a single newline (\n) to the value of the multi-line string.`. Then we also avoid `newline character` as if every other occurrence of `newline` could mean anything else. ;-) https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2935: Any string may be prefixed with the character `r', indicating that it is a {\em raw string}, in which case no escapes or interpolations are recognized. And what does '\r\n' mean when it is encountered in a raw multi-line string literal? Same processing, I'd assume, but it would be a reader-friendly gesture to confirm this suspicion also for raw strings.
https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2877: {\bf multilineString:}`{\escapegrammar \texttt{"""}}' stringContentTDQ* `{\escapegrammar \texttt{"""}}'; On 2017/01/30 10:40:28, eernst wrote: > Might as well fix the triple space issue here. Done. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2878: `{\escapegrammar \code{'}\code{'}\code{'}}' stringContentTSQ* `{\escapegrammar \code{'}\code{'}\code{'}}'; It does exactly that. The `` is the left-quote and '' is the right-quote. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2879: `r' `{\escapegrammar \texttt{"""}}' (\~{} `{\escapegrammar \texttt{"""}}')* `{\escapegrammar \texttt{"""}}'; On 2017/01/30 10:40:28, eernst wrote: > Double space again. Done. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2880: `r' `{\escapegrammar \code{'}\code{'}\code{'}}' (\~{} `{\escapegrammar \code{'}\code{'}\code{'}}')* `{\escapegrammar \code{'}\code{'}\code{'}}' So again no. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2906: The idea is to ignore whitespace, where whitespace is defined as tabs, spaces and the final newline. These can be represented directly, but since for most characters prefixing by backslash is an identity in a non-raw string, we allow those forms as well. Rewritten to: The idea is to ignore a whitespace-only first line, where ... https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2932: Multiline strings can contain newlines. A carriage return character followed by a newline character is considered a single newline. Newlines in multiline string literals contribute a single newline character to the string value. I have rewritten this a lot - I now use "line break" about what the production NEWLINE defines, and talk about line breaks, not newlines, inside multiline strings. https://codereview.chromium.org/2665613003/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2935: Any string may be prefixed with the character `r', indicating that it is a {\em raw string}, in which case no escapes or interpolations are recognized. I've moved this section above the previous one, so the line-breaking stuff doesn't interrupt the escape stuff. Then it shouldn't be as important to say that line-breaking isn't escape-stuff.
LGTM! Just one typo not addressed, I added the same comment on the new patch set. https://codereview.chromium.org/2665613003/diff/40001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2665613003/diff/40001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2852: However, the use the concatenation operation is still discouraged for efficiency reasons. Instead, the recommended Dart idiom is to use string interpolation. 'the use [of?] the'
https://codereview.chromium.org/2665613003/diff/40001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2665613003/diff/40001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2852: However, the use the concatenation operation is still discouraged for efficiency reasons. Instead, the recommended Dart idiom is to use string interpolation. On 2017/01/31 08:52:47, eernst wrote: > 'the use [of?] the' Done.
Description was changed from ========== Fix specification of multiline strings. Now say that NEWLINE is one of CR, LF or CR+LF, and that all of these contribute a single LF to the string value. Fixes issue #14073 BUG= http://dartbug.com/14073 ========== to ========== Fix specification of multiline strings. Now say that NEWLINE is one of CR, LF or CR+LF, and that all of these contribute a single LF to the string value. Fixes issue #14073 BUG= http://dartbug.com/14073 R=eernst@google.com, johnniwinther@google.com Committed: https://github.com/dart-lang/sdk/commit/ef31637857d03adc302c821906b7772ccf86c29d ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as ef31637857d03adc302c821906b7772ccf86c29d (presubmit successful). |
