Fixes PR21597.
Details
Diff Detail
- Build Status
Buildable 630 Build 630: arc lint + arc unit
Event Timeline
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).
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. |
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.
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?
Done.
Do you have commit access, or would you rather I committed it?
Yes, I have the commit access.
Thanks for reviewing!