[libcxx-commits] [PATCH] D116334: [libc++] Remove std::basic_string's base class in ABIv2

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__config:111
+// Remove basic_string common base
+#  define _LIBCPP_ABI_STRING_REMOVE_BASE
 #elif _LIBCPP_ABI_VERSION == 1
----------------
Mordante wrote:
> This looks more consistent with the other ABI macros.
I agree that the name shouldn't involve `REMOVE`; it should describe the new state of affairs, not just a diff against the obsolete state of affairs. FWLIW, I'd have said `_LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS` — `NO_STRING_BASE` just seems too short/vague. But I acknowledge I'm suffering from a //little// bit of "obscure things should have obscure scary names" (the same reason we've written `template<class ...>` in front of templates for 30 years, you know), and `_LIBCPP_ABI_NO_STRING_BASE` //is// consistent with `_LIBCPP_ABI_NO_BINDER_BASES` and `_LIBCPP_ABI_NO_ITERATOR_BASES`, both of which I myself introduced.

Also, I notice that `_LIBCPP_ABI_ALTERNATE_STRING_LAYOUT` already exists. I suspect we //cannot// combine these two ABI flags into one, because that would mess with people who are (unsupportedly) a-la-carte'ing that flag today. But I'd like to hear @ldionne's take on that idea anyway.

For reference, right now our two string-related ABI flags are `_LIBCPP_ABI_ALTERNATE_STRING_LAYOUT` and `_LIBCPP_ABI_STRING_OPTIMIZED_EXTERNAL_INSTANTIATION`. Yet another option for this one would be `_LIBCPP_ABI_STRING_NO_BASE_CLASS`.

TLDR: I'm reasonably happy with @Mordante's suggested edit. ;)


================
Comment at: libcxx/include/string:1724
     void __throw_out_of_range() const {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        __basic_string_common<true>::__throw_out_of_range();
-#else
-        _VSTD::abort();
-#endif
+        _VSTD::__throw_out_of_range("basic_string");
     }
----------------
(1) My understanding is that the `_LIBCPP_NO_EXCEPTIONS` branch needs to remain. Someone who compiles with `-D_LIBCPP_NO_EXCEPTIONS` must not see any calls to `__throw_out_of_range` popping up at link time.

(2) I have the (more nebulous) impression that we call a dedicated library function `__basic_string_common<true>::__throw_out_of_range()` instead of `__throw_out_of_range("basic_string")` on purpose, because `std::string` is used so heavily, and we don't want the user to pay for the 13-byte string literal `"basic_string"` in every single one of their TUs. (This rationale does not apply in lesser-used places such as `deque`.) However, off the top of my head I don't know that we have ever benchmarked this or that anyone really cares.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116334