[PATCH] D93912: [libc++][P1679] add string contains

[Zoe Carver via Phabricator via llvm-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/string:1450
+    constexpr _LIBCPP_INLINE_VISIBILITY
+    bool contains(const value_type* __s) const
+    { return __self_view(data(), size()).contains(__s); }
----------------
ldionne wrote:
> zoecarver wrote:
> > WimLeflere wrote:
> > > Quuxplusone wrote:
> > > > zoecarver wrote:
> > > > > Thoughts on making this one noexcept as well? (I know it's not noexcept in the standard, does anyone know the reason for that?)
> > > > [Lakos Rule.](https://quuxplusone.github.io/blog/2018/04/25/the-lakos-rule/) If `__s` is null, or points to a non-null-terminated array, then this overload has UB; whereas the other two overloads never have UB.
> > > > 
> > > > No useful opinion from me on making this one noexcept. Personally I can never remember whether vendors are allowed to add `noexcept`, or `constexpr`, or neither. I also don't know if libc++ thinks it's OK to add either keyword if it's not mandated by the standard, because that might be a portability trap for users (they rely on its being noexcept and then their code breaks when they switch to libstdc++ or something).
> > > > 
> > > The implementation is based on `find(const char*)` which is not `noexcept` ([[ https://github.com/llvm/llvm-project/blob/master/libcxx/include/string_view#L438 | string_view::find ]]).
> > > So I think it's best to keep it not noexcept.
> > > 
> > > I think a vendor can be more 'strict' than the standard and still be conformant.
> > > In the MS STL they are more strict for example: https://github.com/microsoft/STL/pull/1478#issuecomment-730789583
> > Libc++ adds noexcept to some things that aren't required to have it in the standard. But I don't think we add constexpr. Maybe @mclow.lists or @ldionne can confirm this, though. 
> > 
> > > The implementation is based on find(const char*) which is not noexcept (string_view::find).
> > > So I think it's best to keep it not noexcept.
> > 
> > Well... they're all implemented with `__str_find ` which is unconditionally noexcept so, I think it might make sense. (Given that, it probably also makes sense to mark that `find` function as noexcept, but that's out of scope.) But, we should definitely also make the corresponding `string_view` overload noexpect if we make this one noexecept. 
> > 
> > Separate note: I know the paper suggests using `__self_view::find`, but why not just use `basic_string::find` (which //is// noexecpt, against the standard specification, btw)? 
> Indeed, we've historically added `noexcept` to some things as "extensions", and my understanding is that we're allowed by the Standard to do so: http://eel.is/c++draft/res.on.exception.handling#5. My personal preference would be to avoid adding these unless they add tremendous value, as they make us diverge from the Standard, and could potentially work against the goal of having a "safe" standard library mode (e.g. when you do something bad, we do something customizeable instead of having UB or necessarily terminating).
> 
> We haven't been adding `constexpr` to things not marked as such in the Standard, and the Standard explicitly forbids us from doing it: http://eel.is/c++draft/constexpr.functions. It would be a bad idea anyway due to portability and future-proofness of our implementation anyway.
> 
> 
> My personal preference would be to avoid adding these unless they add tremendous value, as they make us diverge from the Standard, and could potentially work against the goal of having a "safe" standard library mode (e.g. when you do something bad, we do something customizeable instead of having UB or necessarily terminating).

That's fair, but in this case, we're going to necessarily terminate either way. This function is a simple wrapper for `string_view::find` which is a simple wrapper for `__str_find ` which is always noexcept. Hopefully, the compiler will inline this all away, but there's a fair possibility that we're making the codegen a bit worse, and there is absolutely no benefit afaict. I like the idea of a "safe" standard library mode, and while marking this as noexcept isn't moving towards that, I'd argue it's also not really moving away from it. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93912/new/

https://reviews.llvm.org/D93912