Page MenuHomePhabricator

[libcxx] re.results.form: Format out-of-range subexpression references as null
ClosedPublic

Authored by dexonsmith on Jan 22 2016, 9:17 AM.

Details

Summary

Rather than crashing in match_results::format() when a reference to a
marked subexpression is out of range, format the subexpression as
empty (i.e., replace it with an empty string).

The standard doesn't require that marked subexpression references are
valid (the only preconditions are that ready() == true and that Out is
an output iterator), so the implementation shouldn't segfault on
out-of-range marked subexpressions. The most user-friendly behaviour
(and the easiest to implement) is to treat them as "null" sub-matches.
Another option that I rejected is to leave them un-substituted.

I'm not sure about my choice of where to put the tests. I considered
duplicating them across all four form*.pass.cpp, but thought I'd get
some feedback first.

Diff Detail

Event Timeline

dexonsmith updated this revision to Diff 45692.Jan 22 2016, 9:17 AM
dexonsmith retitled this revision from to [libcxx] re.results.form: Format out-of-range subexpression references as null.
dexonsmith updated this object.
dexonsmith added reviewers: EricWF, mclow.lists.
dexonsmith added a subscriber: cfe-commits.
mclow.lists edited edge metadata.Jan 25 2016, 8:19 AM

Not crashing is good; but silently returning a "wrong" answer rubs me the wrong way. I'll do some research and get back to you on this, but maybe throwing an exception is a better response.

The code looks fine, though; I'm just not sure it's the right thing to do.

mclow.lists added inline comments.Jan 25 2016, 8:49 AM
include/regex
5390–5391

A better idea.
Replace this code:

__out = _VSTD::copy(__matches_[__i].first, __matches_[__i].second, __out);

with:

__out = _VSTD::copy((*this)[__i].first, (*this)[__i].second, __out);

match_results::operator[] already has the logic to do something with out of range indexes.

5442–5443

Same as above.

test/std/re/re.results/re.results.form/form1.pass.cpp
61

Keep the tests.

Given your follow-up review, and the behaviour of
match_results::operator[], do you still have reservations?

dexonsmith edited edge metadata.

Addressed Marshall's review comments: deferring to match_results::operator[]() rather than duplicating the logic.

Ping.

Marshall, does this look okay?

mclow.lists accepted this revision.Feb 3 2016, 11:16 AM
mclow.lists edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 3 2016, 11:16 AM

Committed in r259682.