Details
- Reviewers
ldionne mclow.lists curdeius - Group Reviewers
Restricted Project - Commits
- rGdea74b282061: [libc++] Add `noexcept` to `string::find` and similar members.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
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.
libcxx/include/string | ||
---|---|---|
1387 | It seems that you haven't tested this change. |
libcxx/include/string | ||
---|---|---|
1387 | Good catch, thanks. Updated. |
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.
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.
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.
Aha! Thanks for the help, @curdeius! I didn't realize the link to the status is in the build logs.
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.