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

[Louis Dionne via Phabricator via llvm-commits]

ldionne 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); }
----------------
mclow.lists wrote:
> zoecarver wrote:
> > 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. 
> My personal preference would be to add `NOEXCEPT` here. All this does is call a noexcept constructor, and then calls a noexcept member fn, and returns a bool (which is a noexcept operation as well.
> 
> libc++ has traditionally been quite liberal in marking things NOEXCEPT; it can help codegen tremendously in a few very specific circumstances. 
Yeah, okay, I'm swayed. I think I agree. Feel free to add `noexcept` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93912