This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Support std::regex_constants::match_not_null
ClosedPublic

Authored by timshen on Oct 14 2016, 12:22 AM.

Event Timeline

timshen updated this revision to Diff 74621.Oct 14 2016, 12:22 AM
timshen retitled this revision from to [libcxx] Support std::regex_constants::match_not_null.
timshen updated this object.
timshen added reviewers: mclow.lists, EricWF.
timshen added a subscriber: cfe-commits.
mclow.lists edited edge metadata.Oct 15 2016, 7:04 PM

I like the fix. :-)

However, I think that the test, rather than going in a bug specific file (pr21597.pass.cpp), should be added to the existing tests - where it should have been in the first place. (If this test had been there in the first place, we would have realized that this feature didn't work)

Also, putting the test in as test/std/re/re.const/re.matchflag/match_not_null.pass.cpp might cause someone to look at the contents of that directory and say "Crap! We're missing tests for match_not_bow, match_not_eow, match_any, match_continuous and match_prev_avail as well" (and probably others).

mclow.lists added inline comments.Oct 17 2016, 6:29 AM
libcxx/test/std/re/re.alg/re.alg.search/pr21597.pass.cpp
12 ↗(On Diff #74621)

Rather than this comment about regex_search, there should be a comment about match_not_null, which is really what we're testing here.

Something like:

// match_not_null:
//     The expression shall not match an empty sequence.
timshen updated this revision to Diff 74894.Oct 17 2016, 1:29 PM
timshen marked an inline comment as done.
timshen edited edge metadata.

Updated file location and documentation.

I like the fix. :-)

However, I think that the test, rather than going in a bug specific file (pr21597.pass.cpp), should be added to the existing tests - where it should have been in the first place. (If this test had been there in the first place, we would have realized that this feature didn't work)

Agree.

Also, putting the test in as test/std/re/re.const/re.matchflag/match_not_null.pass.cpp might cause someone to look at the contents of that directory and say "Crap! We're missing tests for match_not_bow, match_not_eow, match_any, match_continuous and match_prev_avail as well" (and probably others).

Making people realize the fact that we don't have tests for match_not_bow, etc. isn't bad, is it? Hopefully it motivates people to add tests. So I'll move the file to re.const/re.matchflag/match_not_null.pass.cpp for now.

BTW, re.const/re.matchflag/match_flag_type.pass.cpp contains match_not_bow, but it doesn't actually test the functionality.

BTW, re.const/re.matchflag/match_flag_type.pass.cpp contains match_not_bow, but it doesn't actually test the functionality.

It tests that it exists and that it has a sane value - which are a good things to test.
But there are no tests that show that when passed to regex_search it does the right thing.

Do we need to test calling regex_match with match_not_null?
If not, then I think this is good to go.

Do you have commit access, or would you rather I committed it?

timshen updated this revision to Diff 75186.Oct 19 2016, 11:52 AM

Add tests for regex_match.

Do we need to test calling regex_match with match_not_null?
If not, then I think this is good to go.

Done.

Do you have commit access, or would you rather I committed it?

Yes, I have the commit access.

Thanks for reviewing!

mclow.lists accepted this revision.Oct 19 2016, 4:15 PM
mclow.lists edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 19 2016, 4:15 PM
This revision was automatically updated to reflect the committed changes.