C++23 string contains implementation and tests
Paper: https://wg21.link/P1679R3
Standard (string): https://eel.is/c++draft/string.contains
Standard (string_view): https://eel.is/c++draft/string.view.ops#lib:contains,basic_string_view
Differential D93912
[libc++][P1679] add string contains WimLeflere on Dec 29 2020, 12:11 PM. Authored by
Details
C++23 string contains implementation and tests Paper: https://wg21.link/P1679R3
Diff Detail
Event Timeline
Comment Actions You might want to wait for D93383 to land, so that it updates C++2a to C++20. In other words, please be patient.
Comment Actions Applied review comments
Comment Actions The macro should be added to libcxx/utils/generate_feature_test_macro_components.py, then run the script to generate the proper macros and tests.
Comment Actions Thanks for your contribution! I think you should try rebasing against main, as most of the changes @curdeius was discussing have been checked in now (thanks Marek!). I believe that should remove the need for most of the boilerplate/generated stuff in this patch, which should end up being much simpler. I see you added tests and updated the synopsis, that's great. Comment Actions The bulk of the changes are coming from introducing C++2b in the generate_feature_test_macro script. Should introducing C++2b in the generate_feature_test_macro script be done in a separate review? The patch was created after rebasing on 1a2eaeb.
Comment Actions Please reupload the diff setting the repo to rG LLVM Monorepo so that the CI gets triggered. You might need to do it twice to make it work. Otherwise use arcanist and it should do the job. Comment Actions Great.
Comment Actions
Comment Actions
Comment Actions A few small comments. This is looking really good :)
Comment Actions @Quuxplusone Arthur, did you still have objections? If not, please accept and I'll commit this.
Comment Actions @ldionne: LGTM except that the non-constexpr tests no longer need to return true; from bool test() — they could do void test() for now. The current direction on constexpr/noexcept sounds great to me.
Comment Actions Thanks Arthur. Indeed, this doesn't apply cleanly on top of main. Please rebase and address Arthur's comments. Requesting changes just so it shows up in the review queue, but this LGTM except for what I just said.
Comment Actions Ok, I take it back. __str_find calls Traits::find, which is NOT always noexcept. (It *is* for all the specializations of std::char_traits, but that's not enough). So we can't slap NOEXCEPT here; in fact, we should revisit __str_find, and remove some of the NOEXCEPTs that are already there. Comment Actions The entry for __cpp_lib_string_contains in generate_feature_test_macro_components.py was already added in D94227, but I can rebase anyway. https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations seems out of date anyway, the draft standard is maybe a better source? Comment Actions
Good call. I had not thought to check https://eel.is/c++draft/version.syn when I did D93830. On the one hand version.syn is more authoritative; on the downside version.syn fails to mention what P-numbered papers each macro refers to, and also thus fails to include a "history" of how each macro should be set in each different language mode. I continue to think that at some point we'll need to rejigger generate_feature_test_macro_components.py to include P-numbered papers (e.g. "p1679") in the big data table, as well as language modes (e.g. "c++14"). I wonder if WG21 considers https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations to be unmaintained/obsolete, now that version.syn is part of the formal standard. Comment Actions Rebased and removed unused bool return from basic_string tests. It's unclear to me if I should add noexcept to the const char* overloads. Comment Actions
I think @mclow.lists is right. We shouldn't have noexcept here, and we also need to remove it from some of the __str_find overloads (as a separate patch, which I'm happy to make). Comment Actions @WimLeflere Don't add noexcept. Your patch is fine as-is -- we'll tackle the noexcept story separately. Hmm, indeed. std::char_traits::find is marked as constexpr since C++17, but technically it could still throw at runtime. @zoecarver Feel free to submit a patch to remove it if you want. You could also audit the other helper functions (like __str_rfind - I see there's a bunch) for the same problem. Comment Actions @WimLeflere I just noticed that during review we didn't mention the https://libcxx.llvm.org/docs/Cxx2bStatus.html should be updated. Does this commit fully implement the paper? Once you confirm whether it's completed or partly done I'll update the status page. Comment Actions The paper is fully implemented. With the caveat that std::string::contains is not constexpr yet. Comment Actions Looking at the code that's only because std::string itself can't be used as constexpr yet. The patch has the proper constexpr. I'll update the status page. Thanks for the information. Comment Actions That's funny because https://en.cppreference.com/w/cpp/compiler_support is already updated! |
Before libc++ starts adding C++23 stuff, I think it needs someone (notme) to do a "C++20 exists now" audit. References to "2a" should be changed to "20" — especially in the test suite, where I keep tripping up by typing --param std=c++20 instead of --param std=c++2a — and then a "2b" mode needs to be added.
I weakly recommend using "2b" instead of "23" here, for pedantic accuracy. I admit that it is very likely that C++2b will be C++23, and that by recommending "2b" I'm just making work for somebody three years from now. :P