Page MenuHomePhabricator

Add `noexcept` to `string::find` and similar members.
ClosedPublic

Authored by zoecarver on Feb 1 2021, 10:15 PM.

Details

Summary

Adds noexcept to string_view/string::find and similar members
(rfind, etc.). See discussion in D95251. Refs D95821.

Diff Detail

Event Timeline

zoecarver requested review of this revision.Feb 1 2021, 10:15 PM
zoecarver created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2021, 10:15 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver retitled this revision from [libc++] Add `noexcept` to `string::find` and similar members. to Add `noexcept` to `string::find` and similar members..Feb 1 2021, 10:16 PM
curdeius added a subscriber: curdeius.EditedFeb 2 2021, 5:11 AM

Shouldn't it be tested with ASSERT_NOEXCEPT or rather with its libc++-specific equivalent (LIBCPP_ASSERT_NOEXCEPT)?

Shouldn't it be tested with ASSERT_NOEXCEPT or rather with its libc++-specific equivalent (LIBCPP_ASSERT_NOEXCEPT)?

Hmm, as far as I can tell, currently we don't test whether these members are noexcept, but that also doesn't mean we shouldn't. If I do add it, I'll put it under the libcxx/ directory (and I should probably test the other overloads as well). I don't really like the idea of testing something this simple, I'm not sure how much we gain from that, especially when the standard doesn't require it. What's the case where one of these tests would fail?

Shouldn't it be tested with ASSERT_NOEXCEPT or rather with its libc++-specific equivalent (LIBCPP_ASSERT_NOEXCEPT)?

Hmm, as far as I can tell, currently we don't test whether these members are noexcept, but that also doesn't mean we shouldn't. If I do add it, I'll put it under the libcxx/ directory (and I should probably test the other overloads as well). I don't really like the idea of testing something this simple, I'm not sure how much we gain from that, especially when the standard doesn't require it. What's the case where one of these tests would fail?

Thinking about it more. I think we do try to test noexcept specifiers in other places, so it probably does make sense to test these as well. I'll add tests for the changed functions and other overloads.

zoecarver updated this revision to Diff 320883.Feb 2 2021, 12:38 PM
  • Add LIBCPP_ASSERT_NOEXCEPT tests.
curdeius added inline comments.Feb 2 2021, 12:57 PM
libcxx/include/string
1387

It seems that you haven't tested this change.
Should be somewhere here: https://github.com/llvm/llvm-project/tree/main/libcxx/test/std/strings/basic.string/string.ops/string_compare.

zoecarver added inline comments.Feb 2 2021, 3:16 PM
libcxx/include/string
1387

Good catch, thanks. Updated.

zoecarver updated this revision to Diff 320926.Feb 2 2021, 3:16 PM
  • Add tests for string::compare
curdeius accepted this revision.Feb 3 2021, 9:20 AM

LGTM.

ldionne requested changes to this revision.Feb 5 2021, 8:29 AM

Can we mark the functions we added noexcept on as such in the synopsis, and add // noexcept as an extension? I think it's useful to track those. Apart from that, this LGTM.

This revision now requires changes to proceed.Feb 5 2021, 8:29 AM
zoecarver updated this revision to Diff 321841.Feb 5 2021, 11:36 AM
  • Mark noexcept in synopsis
ldionne accepted this revision.Feb 5 2021, 11:53 AM

LGTM, please ship once CI passes. Thanks!

We should also look into marking some more char_traits operations as noexcept if they are not and if we're allowed. Otherwise, calling a non-noexcept function from a noexcept function forces the compiler to generate code that looks like:

try { f(); } catch (...) { std::terminate(); }

which isn't great.

This revision is now accepted and ready to land.Feb 5 2021, 11:53 AM

LGTM, please ship once CI passes. Thanks!

CI's been "going" for 90 minutes now... is there any way to see what the status is or if it's even started?

@zoecarver, https://buildkite.com/llvm-project/libcxx-ci/builds/1294
To get there just click on "pre-merge checks", then the non-clickable link in the log: https://buildkite.com/llvm-project/diff-checks/builds/28054.
Then just follow the tasks.
Looks green, just Mac builds still waiting for an agent.

@zoecarver, https://buildkite.com/llvm-project/libcxx-ci/builds/1294
To get there just click on "pre-merge checks", then the non-clickable link in the log: https://buildkite.com/llvm-project/diff-checks/builds/28054.
Then just follow the tasks.
Looks green, just Mac builds still waiting for an agent.

Aha! Thanks for the help, @curdeius! I didn't realize the link to the status is in the build logs.