[Parse] An implementation for SE-0182 #11080

johnno1962 · 2017-07-20T14:25:51Z

@milseman, here is an implementation of SE-0182 - https://github.com/apple/swift-evolution/blob/7c84029d89163dd65bdd4766e8aea0aa398b12fe/proposals/0182-newline-escape-in-strings.md. It is complete and tested except for checking whether the \ escape is used on the last line which is, TBH, hard to implement and a no-op anyway. If you have any other suggesttions for tests let me know. I think the two tests I added exercise all code paths in the changes to the implementation.

milseman · 2017-07-20T21:05:27Z

@swift-ci please test

milseman

Overall looks good. I would like some more tests, and maybe some weigh-in from @jrose-apple or @nkcsgexi

milseman · 2017-07-20T21:15:38Z

lib/Parse/Lexer.cpp

+    }
+  }
+  return false;
+}


I realize much of the Lexer is inconsistent in this usage, but Clang's CharInfo.h (which is used in this file) does provide isWhitespace, isHorizontalWhitespace and isVerticalWhitespace.

@jrose-apple or @nkcsgexi or anyone who would know better, is it desirable to use these instead?

Similarly for the rest of this patch

difficult to use in a switch the way things are coded

milseman · 2017-07-20T21:28:03Z

test/Parse/multiline_string.swift

+// CHECK: "substring1 "
+// CHECK: "substring2 substring3"
+
+_ = """


Not sure if this is tested here, but is all whitespace after the \ skipped?

Also, we should explicitly test behavior when non-whitespace follows the backslash, including comments, as an error.

all whitespace skipped. I’ll add an error on non-whitespace test

… which reveals a bug :)

jrose-apple

Not so familiar with Parse these days, so deferring to Xi and Rintaro.

jrose-apple · 2017-07-20T23:13:38Z

lib/Parse/Lexer.cpp

@@ -1189,6 +1189,27 @@ unsigned Lexer::lexUnicodeEscape(const char *&CurPtr, Lexer *Diags) {
  return CharValue;
 }

+/// isElidedNewlineEscape - Check for valid elided newline escape and
+/// move pointer passed in to the character after the end of the line.
+static bool isElidedNewlineEscape(const char *&CurPtr, ssize_t Offset) {


I'm unhappy with a function named like a predicate having side effects. How about something like maybeConsumeNewlineEscape?

Fair enough, renamed.

johnno1962 · 2017-07-20T23:51:35Z

Toolchain uploaded for test drive: http://johnholdsworth.com/swift-LOCAL-2017-07-20-a-osx.tar.gz

xwu · 2017-07-21T00:00:45Z

lib/Parse/Lexer.cpp

@@ -1921,6 +1952,12 @@ StringRef Lexer::getEncodedStringSegment(StringRef Bytes,
    case '\'': TempString.push_back('\''); continue;
    case '\\': TempString.push_back('\\'); continue;

+    case ' ': case '\t': case '\n': case '\r':
+      if(maybeConsumeNewlineEscape(BytesPtr, -1)) {


Nit: insert space after if :)

Ha! There had to be something :)

rintaro · 2017-07-21T00:28:21Z

test/Parse/multiline_string.swift

+  """
+
+// CHECK: "trailing "
+// CHECK: "substring1 "


This shouldn't be "substring1 " (3 space) ?

How do you mean?

Ah my bad, please ignore this comment.

rintaro · 2017-07-21T00:33:14Z

test/Parse/multiline_string.swift

+    \("""
+      substring2 \          
+      substring3
+      """)\


Rationale for SE-182 states:

It also discussed whether \ should be allowed on the line immediately following the close """ and agreed that it was best to not allow it in this go-around.

This \ should be rejected.

This looks like a mistake in the updated proposal. It should read “immediately preceding”.

This is not implemented as an error.

Ah, sorry. I didn't read the PR comment.

rintaro · 2017-07-21T00:46:00Z

Could you add tests for these cases?

_ = """
    foo\ ↵
↵
    bar↵
    """ // -> "foo\nbar"

_ = """
    foo\ ↵
    ↵
    bar↵
    """ // -> "foo\nbar"

rintaro · 2017-07-21T00:54:03Z

Also:

_ = """
    foo \
      bar
    """ // -> "foo   bar"

rintaro · 2017-07-21T01:19:18Z

test/Parse/multiline_errors.swift

+  line one \ non-whitepace
+  line two
+  """
+  // expected-error@-3 {{Invalid escape sequence in literal}}


nit: "invalid not "Invalid

rintaro · 2017-07-21T02:12:46Z

lib/Parse/Lexer.cpp

+        case ' ': case '\t': case '\n': case '\r':
+          if (MultilineString && maybeConsumeNewlineEscape(CurPtr, -1))
+            continue;
+          LLVM_FALLTHROUGH;


Maybe not a big problem, but this regress the diagnostics for "foo \("\ ")"

test.swift:1:21: error: unterminated string literal let a = "foo \("\ ")" ^ test.swift:1:20: error: consecutive statements on a line must be separated by ';' let a = "foo \("\ ")" ^ ; test.swift:1:20: error: expected expression let a = "foo \("\ ")" ^

I think this would be sufficient:

case '\n': case '\r': if (AllowNewline.back()) continue; LLVM_FALLTHROUGH; case 0: // Don't jump over newline/EOF due to preceding backslash! return CurPtr-1;

Well spotted! fixed

rintaro · 2017-07-21T04:54:37Z

LGTM, deferring to @nkcsgexi

xwu · 2017-07-21T05:40:26Z

lib/Parse/Lexer.cpp

+              escapedChar == ' ' || escapedChar == '\t')
+            continue;
+          LLVM_FALLTHROUGH;
+        case 0:
          // Don't jump over newline/EOF due to preceding backslash!


This comment needs to be updated, methinks.

johnno1962 · 2017-07-21T12:22:17Z

Updated toolchain toolchain: http://johnholdsworth.com/swift-LOCAL-2017-07-21-a-osx.tar.gz

DougGregor · 2017-07-25T21:04:57Z

@swift-ci please smoke test

johnno1962 · 2017-07-26T10:31:22Z

Thanks @DougGregor, @milseman, @rintaro, @xwu

milseman reviewed Jul 20, 2017

View reviewed changes

jrose-apple requested a review from rintaro July 20, 2017 22:01

jrose-apple reviewed Jul 20, 2017

View reviewed changes

xwu reviewed Jul 21, 2017

View reviewed changes

rintaro reviewed Jul 21, 2017

View reviewed changes

rintaro requested a review from nkcsgexi July 21, 2017 04:54

xwu reviewed Jul 21, 2017

View reviewed changes

johnno1962 changed the title ~~An implementation for SE-0182~~ [Parse] An implementation for SE-0182 Jul 21, 2017

[Parse] An implementation for SE-0182

c0fcc1a

DougGregor merged commit 6d06792 into swiftlang:master Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Parse] An implementation for SE-0182 #11080

[Parse] An implementation for SE-0182 #11080

johnno1962 commented Jul 20, 2017 •

edited

Loading

milseman commented Jul 20, 2017

milseman left a comment

milseman Jul 20, 2017

johnno1962 Jul 20, 2017

milseman Jul 20, 2017

johnno1962 Jul 20, 2017

johnno1962 Jul 20, 2017

jrose-apple left a comment

jrose-apple Jul 20, 2017

johnno1962 Jul 20, 2017

johnno1962 commented Jul 20, 2017

xwu Jul 21, 2017

johnno1962 Jul 21, 2017

rintaro Jul 21, 2017 •

edited

Loading

johnno1962 Jul 21, 2017

rintaro Jul 21, 2017

rintaro Jul 21, 2017

johnno1962 Jul 21, 2017

johnno1962 Jul 21, 2017

rintaro Jul 21, 2017

rintaro commented Jul 21, 2017 •

edited

Loading

rintaro commented Jul 21, 2017

rintaro Jul 21, 2017

johnno1962 Jul 21, 2017

rintaro Jul 21, 2017 •

edited

Loading

rintaro Jul 21, 2017

johnno1962 Jul 21, 2017

rintaro commented Jul 21, 2017

xwu Jul 21, 2017

johnno1962 commented Jul 21, 2017

DougGregor commented Jul 25, 2017

johnno1962 commented Jul 26, 2017

[Parse] An implementation for SE-0182 #11080

[Parse] An implementation for SE-0182 #11080

Conversation

johnno1962 commented Jul 20, 2017 • edited Loading

milseman commented Jul 20, 2017

milseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrose-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnno1962 commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro commented Jul 21, 2017 • edited Loading

rintaro commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro commented Jul 21, 2017

Choose a reason for hiding this comment

johnno1962 commented Jul 21, 2017

DougGregor commented Jul 25, 2017

johnno1962 commented Jul 26, 2017

johnno1962 commented Jul 20, 2017 •

edited

Loading

rintaro Jul 21, 2017 •

edited

Loading

rintaro commented Jul 21, 2017 •

edited

Loading

rintaro Jul 21, 2017 •

edited

Loading