Page MenuHomePhabricator

[FIX][libc++][Regex] Using regex_constants match_prev_avail | match_not_bol | match_not_bow
ClosedPublic

Authored by dnsampaio on Mar 4 2020, 9:15 AM.

Details

Summary

pr42199
When using regex_constants::match_prev_avail, it is defined that
--first is valid, and match_not_bol and match_not_bow should be
ignored. At the moment these flags are not ignored. This fixis that.

Diff Detail

Event Timeline

dnsampaio created this revision.Mar 4 2020, 9:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Mar 11 2020, 8:13 PM
EricWF added inline comments.
libcxx/test/std/re/re.const/re.matchflag/match_prev_avail.pass.cpp
17

Please fix the FIXME.

37

Why are these commented out?

This revision now requires changes to proceed.Mar 11 2020, 8:13 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 11 2020, 8:13 PM

Hi @EricWF, thanks for the review.
Indeed there seems to have some other unrelated issue for matching the word boundary \b. Perhaps I wrongly assumed that it should also match line beginning and end.
For example, I would expect all these matches to succeed:

cout << regex_match("a", regex("\ba")) << "\n"
     << regex_match("a", regex("a\b")) << "\n"
     << regex_match("a", regex("\ba\b")) << "\n"
     << regex_match("a", regex("a")) << "\n";

But what I get is:

0
0
0
1

So it is unrelated in how match_not_bow is ignored when using match_prev_avail. I would rather open a defect for that apart.

dnsampaio marked an inline comment as done.Mar 13 2020, 7:24 PM
dnsampaio updated this revision to Diff 251991.Mar 23 2020, 4:52 AM
  • Fixed tests to use "\\b" instead of "\b".
dnsampaio marked an inline comment as done.Mar 23 2020, 4:53 AM

Gentle Ping. I believe all issues with this fix have been fulfilled.

Ping. I would like to move along this small patch.

dnsampaio edited the summary of this revision. (Show Details)Apr 27 2020, 8:59 AM
miyuki accepted this revision.Apr 30 2020, 9:22 AM

The updated patch LGTM

ldionne accepted this revision.Apr 30 2020, 2:42 PM

Thanks for your patience. I'm fine with this. @EricWF Please post-commit review this if you still have issues with it.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 30 2020, 3:40 PM
This revision was automatically updated to reflect the committed changes.

@dnsampaio

This seems to have broken the ASAN bot. Could you please take a quick look?

http://lab.llvm.org:8011/builders/libcxx-libcxxabi-x86_64-linux-ubuntu-asan/builds/3207/steps/test.libcxx/logs/FAIL%3A%20libc%2B%2B%3A%3Amatch_prev_avail.pass.cpp

Also pasted at https://gist.github.com/ldionne/b85d28805bdf1550b0b59a96146f4b81 cause I don't know when the build bot logs go away.

Hi, @ldionne
I just submitted a what I think is the test fix. It was the only place I was not using the second element of the string and could generate a underflow.

Hi, @ldionne
I just submitted a what I think is the test fix. It was the only place I was not using the second element of the string and could generate a underflow.

Thanks for taking a look!