<div dir="ltr"><div dir="ltr">On Sat, Nov 16, 2019 at 8:59 AM Eric Fiselier via libcxx-commits <<a href="mailto:libcxx-commits@lists.llvm.org" target="_blank">libcxx-commits@lists.llvm.org</a>> wrote:<br></div><div dir="ltr"><br></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div>Can you expand on why efficient termination is important?<br></div></div></blockquote><div><br></div><div>In the commit message I meant "fast" as in "fail fast"; the actual time taken for the process to terminate is not super important to us (Chromium). But we do care about object code size quite a bit. I think (perhaps I am wrong) that __builtin_trap gets us smaller code than std::exit.</div><div><br></div><div>Also, just from poking around, it seems that _LIBCPP_DEBUG=0 does not turn on asserts in all places. Random example from ./libcxx/test/std/strings/basic.string/string.iterators/db_iterators_7.pass.cpp:</div><div><br></div><div>```</div><div>#if _LIBCPP_DEBUG >= 1<br><br>#define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : std::exit(0))<br></div><div>```</div><div><br></div><div>I.e. 0 would not do it.</div><div><br></div><div>And, as far as I can tell, _LIBCPP_DEBUG only takes effect in files under libcxx/test/ — I take it this means the defines do nothing in production release builds. What I want is this callee hardening in production builds. If I understand correctly — and I'm still super new to this code base — the _LIBCPP_ASSERT calls are where I want them in production, and with _LIBCPP_HARDEN defined, they'll do their thing.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div></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. <br></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></blockquote><div><br></div><div>So should I update the patch to change the definition of __libcpp_debug_function when _LIBCPP_HARDEN is defined?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"> </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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></blockquote><div><br></div><div>Did that, yep.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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></blockquote><div><br></div><div>So should I go back to the previous ,-expression implementation? :)</div><div><br></div><div>Thanks for the review!</div></div></div>
</div>