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

[Jan Wilken Dörrie via Phabricator via libcxx-commits]

jdoerrie marked an inline comment as done.
jdoerrie added a comment.

In D70343#2034645 <https://reviews.llvm.org/D70343#2034645>, @palmer wrote:

> Here are some binary size numbers, from a Chromium release build. Here is the build configuration (likely meaningful only to Chromium people; but this basically means "fully optimized except for the final PGO phase"):
>
> ~/chromium/src $ cat out/Release/args.gn
>  is_debug = false
>  is_official_build = true
>  is_asan = false
>  use_goma = true
>  enable_nacl = false
>  is_component_build = false
>
> origin/master:
>  -rwxr-xr-x 1 palmer primarygroup 162973344 May 12 17:30 out/Release/chrome
>
> With libcxx-debug-level-0:
>  -rwxr-xr-x 1 palmer primarygroup 169825792 May 12 18:20 out/Release/chrome
>  Cost: 6,852,448 (4.2% of 162,973,344)
>
> With libcxx-debug-level-0 with __builtin_trap instead of printing:
>  -rwxr-xr-x 1 palmer primarygroup 167307088 May 13 10:08 out/Release/chrome
>  Cost: 4,333,744 (2.6% of 162,973,344)
>
> So the cost is still high, but significantly less with __builtin_trap. So I think this mode is valuable.


Thanks for the analysis, I agree that those args.gn make sense :)



================
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];
+    }
----------------
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.


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

https://reviews.llvm.org/D70343