This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Fix match_results for alternatives
ClosedPublic

Authored by K-ballo on Jan 21 2015, 3:47 PM.

Details

Summary

Initialize submatch results to unmatched. They will not be modified for failed alternatives (they might not even be looked at), and would otherwise leave the pair of iterators value-initialized instead of pointing to the end of the searched sequence. Fixes PR22061.

Diff Detail

Event Timeline

K-ballo updated this revision to Diff 18563.Jan 21 2015, 3:47 PM
K-ballo retitled this revision from to [libcxx] Fix match_results for alternatives.
K-ballo updated this object.
K-ballo edited the test plan for this revision. (Show Details)
K-ballo added reviewers: mclow.lists, EricWF.
K-ballo added a subscriber: Unknown Object (MLST).
mclow.lists edited edge metadata.Jan 23 2015, 9:13 AM

The fix looks reasonable, it fixes the OP's problem, and the new test fails w/o the fix.

However, the change to <regex> is in two places, and I don't see how this test exercises both code paths.

Other than that (and the nits in the test) this looks good.

test/std/re/re.results/re.results.acc/index.pass.cpp
27

How about assert(m.size() == 6) here? - or >5 since we're checking elements 0..5

50

Should this be m[5] ?

The fix looks reasonable, it fixes the OP's problem, and the new test fails w/o the fix.

However, the change to <regex> is in two places, and I don't see how this test exercises both code paths.

Good point, I'll parametrize the test and invoke it for both ECMAScript and extended POSIX.

test/std/re/re.results/re.results.acc/index.pass.cpp
27

assert(m.size() == 4) actually, the main match plus three subexpressions. Any out of range access shall return an unmatched result, and the original test was exercising that.

50

Indeed, good catch.

K-ballo updated this revision to Diff 18688.Jan 23 2015, 11:55 AM
K-ballo edited edge metadata.

Address review comments.

mclow.lists accepted this revision.Jan 24 2015, 12:48 PM
mclow.lists edited edge metadata.
mclow.lists added inline comments.
test/std/re/re.results/re.results.acc/index.pass.cpp
28

Ok, then I'm really confused.

The code for match_results::operator[] is:

return __n < __matches_.size() ? __matches_[__n] : __unmatched_;

So how does the change in <regex> affect this result?

This revision is now accepted and ready to land.Jan 24 2015, 12:48 PM
K-ballo added inline comments.Jan 24 2015, 4:40 PM
test/std/re/re.results/re.results.acc/index.pass.cpp
28

The changes affect unmatched alternative subexpressions. Consider instead the expression "(z)|cd((e)fg)hi", the (z) subexpression would then be m[1] instead of m[3]. In both cases the subscript is within range, so the out-of-range __unmatched_ result is not referenced.

K-ballo closed this revision.Jan 28 2015, 2:45 PM

SVN r227384