[libcxx-commits] [PATCH] D118733: [libc++] Remove the std::string base class

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/src/string.cpp:26-38
+template <bool>
+struct __basic_string_common;
+
+template <>
+struct __basic_string_common<true> {
+    // Both are defined in string.cpp
+    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const {
----------------
I suggest this instead:

```
template <bool>
struct __basic_string_common {
    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const;
    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
};

void __basic_string_common<true>::__throw_length_error() const {
    _VSTD::__throw_length_error("basic_string");
}

void __basic_string_common<true>::__throw_out_of_range() const {
    _VSTD::__throw_out_of_range("basic_string");
}
```

Also, I suggest we rename `_LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS` to `_LIBCPP_ABI_DONT_EXPORT_BASIC_STRING_COMMON` or something along those lines. Basically, the ABI breaking change here would only be that we're not exporting the functions anymore from the dylib.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118733