Closed Bug 615070 Opened 9 years ago Closed 9 years ago

Newline after backslash is invalid in regexp literals

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

One of the last remaining issues that are really easy to fix. The following code is invalid according to ES5 7.8.5 (see RegularExpressionNonTerminator)

/abc\
/
Attached patch Patch (obsolete) — Splinter Review
Also added a LexicalConventions test directory like in ecma_3. I find it hard to come up with a concise comment or commit message for this change, feedback appreciated.
Attachment #493537 - Flags: review?(jwalden+bmo)
Summary: Newline after backslash in regexp literals should be a syntax error → Newline after backslash is invalid in regexp literals
Comment on attachment 493537 [details] [diff] [review]
Patch

(In reply to comment #1)
> Also added a LexicalConventions test directory like in ecma_3.

Please put in ecma_5/RegExp, since regular expression syntax is what you're testing.

> I find it hard to come up with a concise comment or commit message for this
> change, feedback appreciated.

Nothing wrong with using the bug summary if it's accurate, seems to me (and if it isn't I generally back-fix the bug summary to what the commit summary was).  I don't agonize much over commit messages -- accurately give the problem (or at least its general flavor), then let the bug reference do the rest.

I could live with the test location if it were the only tweak desired.  But I still see two other problems, one large, one a bit smaller.  First, you've added a browser.js file for a new directory, but you haven't added the corresponding shell.js.  Maybe that works for browser-based test runs, but it doesn't work at all for command-line test runs.  (python tests/jstests.py path/to/js ecma_5/RegExp for example.)  Moving the test will, as a side benefit, eliminate this problem.  Second, seeing as that condition checks for both \n and for EOF, I think we should have at least one test for the EOF case as well (which, incidentally, looks like it currently over-gets for backslash in regexp literal followed by EOF).
Attachment #493537 - Flags: review?(jwalden+bmo) → review-
Attached patch Patch v2Splinter Review
(In reply to comment #2)
> First, you've
> added a browser.js file for a new directory, but you haven't added the
> corresponding shell.js.

I did, but apparently Bugzilla's diff viewer does not show new files that are empty...

> Second, seeing as that condition checks for
> both \n and for EOF, I think we should have at least one test for the EOF case
> as well (which, incidentally, looks like it currently over-gets for backslash
> in regexp literal followed by EOF).

True, added some tests for backslash followed by EOF. Also added tests for EOF/newline in a character class.
Attachment #493537 - Attachment is obsolete: true
Attachment #493727 - Flags: review?(jwalden+bmo)
Comment on attachment 493727 [details] [diff] [review]
Patch v2

(In reply to comment #3)
> I did, but apparently Bugzilla's diff viewer does not show new files that are
> empty...

Oh, right.  *eggface*


> True, added some tests for backslash followed by EOF. Also added tests for
> EOF/newline in a character class.

Excellent.  And thanks for putting up with my nitpicking!
Attachment #493727 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/mozilla-central/rev/651d80c9ad15
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 617284
So, we just have to put up with long regexps these days? For example:

return selectedText && /^http:\/\/|^https:\/\/|^file:\/\/|\
  ^ftp:\/\/|^about:|^mailto:|^news:|^snews:|^telnet:|^ldap:|\
  ^ldaps:|^gopher:|^finger:|^javascript:/i.test(selectedText);
You can also use the RegExp constructor, which accepts the pattern as string. Like re = new RegExp(s)

Note that the newline after the backslash was treated as a newline in the pattern, which was not very intuitive. Other browsers also reject it, but that's less important for thunderbird I guess ;)
You need to log in before you can comment on or make changes to this bug.