[libcxx-commits] [PATCH] D70343: Add a `_LIBCPP_HARDEN` define

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

I'm a bit confused by the build size numbers you gave compared to the patch.

There's a confusing distinction in libc++ between `_LIBCPP_DEBUG_LEVEL` and `_LIBCPP_DEBUG`. `
`_LIBCPP_DEBUG_LEVEL` is always one more that `_LIBCPP_DEBUG`. So `libcpp-debug-level-0`, shouldn't enable anything,
But I assume you correctly set `_LIBCPP_DEBUG` to zero.

Also, this patch has weird behavior? If I set `_LIBCPP_DEBUG=0` and `_LIBCPP_HARDEN` I don't get the `__builtin_trap` implementation.
I think the idea of hardening and the want to make the assertion cheaper to compile should be dealt with separately.

Could we address the compile time by making `__libcpp_debug_function`  overrideable as a macro?



================
Comment at: libcxx/include/string_view:282
+    const_reference operator[](size_type __pos) const _NOEXCEPT {
+      return _LIBCPP_ASSERT(size() == 0 || __pos < size(), "string_view[] index out of bounds"), __data[__pos];
+    }
----------------
jdoerrie wrote:
> palmer wrote:
> > EricWF wrote:
> > > jdoerrie wrote:
> > > > Checking that `size() == 0` seems like a mistake, as otherwise you would allow arbitrary indices for an empty string_view. This should just be `_LIBCPP_ASSERT(__pos < size(), "...");` unless I'm missing something obvious.
> > > > 
> > > > Given that string_view implies C++17 or newer, you should also be able to use multiple expressions in a constexpr function. IMO the following is easier to read:
> > > > 
> > > > ```
> > > >     _LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY
> > > >     const_reference operator[](size_type __pos) const _NOEXCEPT {
> > > >       _LIBCPP_ASSERT(__pos < size(), "string_view[] index out of bounds");
> > > >       return __data[__pos];
> > > >     }
> > > > ```
> > > > 
> > > `string_view` doesn't imply C++17. We support it in all dialects.
> > So should I switch back to the previous 2-statement form, again? :)
> No need, I believe Eric is referring to my initial 2 line suggestion. The one line solution is required to be compatible with C++11, where constexpr functions could only consist of a single return statement.
That works for me.


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

https://reviews.llvm.org/D70343