Page MenuHomePhabricator

Protect exceptional path under libcpp-no-exceptions
ClosedPublic

Authored by rogfer01 on Oct 31 2016, 2:31 AM.

Details

Summary

This is another followup of D24562

These tests are of the form

try {
   action-that-may-throw
   assert(!exceptional-condition)
   assert(some-other-facts)
 } catch (relevant-exception) {
   assert(exceptional-condition)
 }

Under libcpp-no-exceptions there is still value in verifying
some-other-facts while avoiding the exceptional case. So for these tests
just conditionally check some-other-facts if exceptional-condition is
false. When exception are supported make sure that a true
exceptional-condition throws an exception

Diff Detail

Repository
rL LLVM

Event Timeline

rogfer01 updated this revision to Diff 76362.Oct 31 2016, 2:31 AM
rogfer01 retitled this revision from to Protect exceptional path under libcpp-no-exceptions.
rogfer01 updated this object.
rogfer01 added reviewers: EricWF, mclow.lists, rmaprath.
rogfer01 added a subscriber: cfe-commits.
rmaprath added inline comments.Oct 31 2016, 2:47 AM
test/std/strings/basic.string/string.access/at.pass.cpp
41 ↗(On Diff #76362)

For the cases where an exception should've been thrown, are we not entering the undefined domain at this point?

What if instead, we define two versions of the test() function? one containing the current code as-is, and the other only handles the cases where exceptions are not expected, and we modify the main() function below so that the correct test() case is invoked depending on the presence / absence of exceptions? It's a bit more cumbersome than the current setup, but I'm not totally happy about treading into the undefined domain (if my understanding above is correct).

rogfer01 added inline comments.Oct 31 2016, 3:08 AM
test/std/strings/basic.string/string.access/at.pass.cpp
41 ↗(On Diff #76362)

If I understand this test correctly, it checks for the at member function. While certainly binding a const reference might throw, here it is bound to a lvalue of the same type so no temporary construction should happen.

The original test checks both s.size() and cs.size(). Given that size is a const member function it probably does not matter given that cs and s are aliased, but see comment below.

45 ↗(On Diff #76362)

This is redundant, maybe I should change the if condition to be if (pos < s.size())

rmaprath added inline comments.Oct 31 2016, 3:22 AM
test/std/strings/basic.string/string.access/at.pass.cpp
41 ↗(On Diff #76362)

Right, so it's the at() method which is expected to throw in this case. What you are doing here is carefully avoiding the exception throwing code-path while keeping the remaining assertions intact. Makes sense.

I'll go over the rest of the test cases to double check. But I'm happy with this.

(You need to wait for approval from @EricWF or @mclow.lists to commit)

mclow.lists edited edge metadata.Oct 31 2016, 6:14 PM

This all looks ok to me - all mechanical changes.
I wonder if there's a better way to refactor these - there's all that duplicated code.

Does this look any better to you?
Replace:

try
{
    s.replace(pos1, n1, str, pos2);
    LIBCPP_ASSERT(s.__invariants());
    assert(pos1 <= old_size && pos2 <= str.size());
    assert(s == expected);
    typename S::size_type xlen = std::min(n1, old_size - pos1);
    typename S::size_type rlen = std::min(S::npos, str.size() - pos2);
    assert(s.size() == old_size - xlen + rlen);
}
catch (std::out_of_range&)
{
    assert(pos1 > old_size || pos2 > str.size());
    assert(s == s0);
}

with:

if(pos1 <= old_size && pos2 <= str.size())
{
    s.replace(pos1, n1, str, pos2);
    LIBCPP_ASSERT(s.__invariants());
    assert(s == expected);
    typename S::size_type xlen = std::min(n1, old_size - pos1);
    typename S::size_type rlen = std::min(S::npos, str.size() - pos2);
    assert(s.size() == old_size - xlen + rlen);
}
#ifndef TEST_HAS_NO_EXCEPTIONS
else
{
    try { s.replace(pos1, n1, str, pos2); }
    catch (std::out_of_range&)
    {
        assert(pos1 > old_size || pos2 > str.size());
        assert(s == s0);
    }
}
#endif
test/std/strings/basic.string/string.access/at.pass.cpp
42 ↗(On Diff #76362)

I think you should just test s.size() here, not both s and cs. (the original code got it wrong, too)
The assert at L45 can be removed.

rogfer01 updated this revision to Diff 76532.Nov 1 2016, 2:54 AM
rogfer01 updated this object.
rogfer01 edited edge metadata.

Update tests following Marshall's suggestion to avoid too much code duplication. Also add assert(false) after the throwing action to ensure that the expected exception is actually thrown.

mclow.lists accepted this revision.Nov 1 2016, 7:37 AM
mclow.lists edited edge metadata.

Thanks for making these changes; I think this looks much better than before.
Feature creep: A bunch of the local variables can be marked as const.

test/std/strings/basic.string/string.modifiers/string_insert/size_size_char.pass.cpp
27 ↗(On Diff #76532)

Nit. old_size should be const
(and in a few other places)

test/std/strings/basic.string/string.modifiers/string_replace/size_size_T_size_size.pass.cpp
34 ↗(On Diff #76532)

s0 should be const, too.

This revision is now accepted and ready to land.Nov 1 2016, 7:37 AM
rogfer01 updated this revision to Diff 76566.Nov 1 2016, 8:53 AM
rogfer01 edited edge metadata.

Const-ify variables.

This revision was automatically updated to reflect the committed changes.