<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 15, 2019, 8:27 PM Jan Wilken Dörrie via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">jdoerrie added a comment.<br>
<br>
In D70343#1748522 <<a href="https://reviews.llvm.org/D70343#1748522" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D70343#1748522</a>>, @EricWF wrote:<br>
<br>
> What's the difference between `_LIBCPP_HARDEN` and simply defining `_LIBCPP_DEBUG=0`? That should turn on `_LIBCPP_ASSERT` in the same cases.<br>
<br>
<br>
I believe the intent of `_LIBCPP_HARDEN` is to turn asserts into program terminations. Ideally this happens as quickly as possible with as little overhead as possible. Thus we directly call `__builtin_trap()` instead of going through `_VSTD::__libcpp_debug_function`. Furthermore, I believe we were under the impression that defining `_LIBCPP_DEBUG=0` turns on other checks as well, that we are not necessarily interested in. Please correct me if I'm wrong.</blockquote></div></div><div dir="auto"><br></div><div dir="auto">This patch will turn on all the same _LIBCPP_ASSERT spellings as _LIBCPP_DEBUG=0. When set to '=1' it turns on additional iterator debugging checks. That said, adding a new hardening level for the purposes of security (instead of debugging) is OK by me.</div><div dir="auto"><br></div><div dir="auto">Can you expand on why efficient termination is important?</div><div dir="auto"><br></div><div dir="auto">I've been meaning to add a mechanism to use `__builtin_trap` (or `__builtin_unreachable` when UBSAN is enabled) instead of `__libcpp_debug_function` for various reasons. </div><div dir="auto"><br></div><div dir="auto">However, for the purposes of testing, we need to maintain the `__libcpp_debug_function` and ensure all _LIBCPP_ASSERT's can be made to use it to report failure. </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: libcxx/include/string_view:282<br>
+    const_reference operator[](size_type __pos) const _NOEXCEPT {<br>
+      return _LIBCPP_ASSERT(size() == 0 || __pos < size(), "string_view[] index out of bounds"), __data[__pos];<br>
+    }<br>
----------------<br>
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.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I agree. the size() == 0 check is incorrect. this should do the same bounds checking as at(pos).</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Given that string_view implies C++17 or newer, you should also be able to use multiple expressions in a constexpr function.</blockquote></div></div><div dir="auto"><br></div><div dir="auto">libc++ enable string view in all dialects.</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> IMO the following is easier to read:<br>
<br>
```<br>
    _LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY<br>
    const_reference operator[](size_type __pos) const _NOEXCEPT {<br>
      _LIBCPP_ASSERT(__pos < size(), "string_view[] index out of bounds");<br>
      return __data[__pos];<br>
    }<br>
```<br>
<br>
<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D70343/new/" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D70343/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D70343" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D70343</a><br>
<br>
<br><br>
</blockquote></div></div></div>