Page MenuHomePhabricator

[libc++] ECMAScript IdentityEscape is ambiguous (2584)
Needs ReviewPublic

Authored by zoecarver on Aug 22 2019, 11:29 AM.


Group Reviewers
Restricted Project

This patch fixes 2584. Now the following works:

const std::regex r1("\\z");
assert(std::regex_match("z", r1));

I wasn't sure if the following should also work (see commented out tests):

const std::regex r1("\\z");
assert(std::regex_match("zz", r1));

Diff Detail

Event Timeline

zoecarver created this revision.Aug 22 2019, 11:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
zoecarver retitled this revision from [libc++] Fix std::abs tests to [libc++] ECMAScript IdentityEscape is ambiguous (2584).Aug 22 2019, 11:32 AM
zoecarver edited the summary of this revision. (Show Details)
zoecarver added reviewers: ldionne, mclow.lists.

Friendly ping :)

ldionne requested changes to this revision.Tue, Mar 3, 12:44 PM
ldionne added inline comments.

Use LWG#2584 to make it clear it's a LWG issue


Also, this should be identity escapes instead of identify escape.


Why are those tests commented out?

This revision now requires changes to proceed.Tue, Mar 3, 12:44 PM
zoecarver updated this revision to Diff 250380.Sat, Mar 14, 1:11 PM
zoecarver marked an inline comment as done.
  • rebase off master
  • address review comments
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Mar 14, 1:11 PM

@ldionne thanks for the review! Sorry for the delayed response/update. Last week I was super busy with another project I work on.


I wasn't sure if they should also work (they currently don't because, while the first part is matched, it fails when it sees the extra z). Looking at it again I think that's the correct behavior (not to match).

I'm going to remove it from this patch. If it is supposed to work, I think that's a different issue than the one addressed here.

zoecarver updated this revision to Diff 251761.Fri, Mar 20, 2:16 PM
  • Remove now valid bad escape tests
zoecarver updated this revision to Diff 252743.Wed, Mar 25, 9:39 PM
  • Diff from master (sorry for the bad diff)