[libc++] Remove the std::string base class

[philnik]

Summary

Removing the base class of std::basic_string is not an ABI break, so we can remove any references to it from the header.

[Mordante]

This change breaks ABI stability in the CI, so it needs more work.

libcxx/include/string

686

The comment contradicts the commit message. But the base indeed seems to be empty.

libcxx/src/string.cpp

24

Maybe add a comment here that the struct isn't declared in any header anymore and is only provided here to export the symbols for ABI stability. (It's kind of obvious when you think about it, but can't hurt to explain it for future readers.)

[ldionne]

libcxx/src/string.cpp

26–38

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.

[philnik]

• Address comments

[Mordante]

LGTM!

[ldionne]

Oooooh, I'm trembling to approve this. If my reasoning on vector_base_common isn't right, this is going to break the whole world (basic_string is like the most used type in the library). But yeah, we've thought it out and it seems legit, so let's go.

[philnik]

In D118733#3295008, @ldionne wrote:

Oooooh, I'm trembling to approve this. If my reasoning on vector_base_common isn't right, this is going to break the whole world (basic_string is like the most used type in the library). But yeah, we've thought it out and it seems legit, so let's go.

What could go wrong? The worst would be that we break thousands of people's software silently :D
Do you cherry-pick the commit onto the release branch?

[dim]

FWIW, this breaks compilation of Firefox:

In file included from Unified_cpp_accessible_base1.cpp:20:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/firefox-97.0/accessible/base/TextLeafRange.cpp:11:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/a11y/DocAccessibleParent.h:11:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/ipc/ipdl/_ipdlheaders/mozilla/a11y/PDocAccessibleParent.h:9:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/ipc/ipdl/_ipdlheaders/mozilla/a11y/PDocAccessible.h:24:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/a11y/IPCTypes.h:16:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/GfxMessageUtils.h:30:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/ipc/ProtocolUtils.h:27:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/ipc/MessageChannel.h:23:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/stl_wrappers/stack:64:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/system_wrappers/stack:3:
In file included from /usr/include/c++/v1/stack:105:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/stl_wrappers/deque:64:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/system_wrappers/deque:3:
/usr/include/c++/v1/deque:1854:9: error: call to '__throw_out_of_range' is ambiguous
        _VSTD::__throw_out_of_range("deque");
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/__config:824:15: note: expanded from macro '_VSTD'
#define _VSTD std
              ^
/wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/throw_gcc.h:97:59: note: candidate function
MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_out_of_range(
                                                          ^
/usr/include/c++/v1/stdexcept:264:6: note: candidate function
void __throw_out_of_range(const char*__msg)
     ^

That said, they're doing very naughty things in e.g. https://hg.mozilla.org/mozilla-central/file/tip/memory/mozalloc/throw_gcc.h#l47 :

namespace std {

// NB: user code is not supposed to touch the std:: namespace.  We're
// doing this after careful review because we want to define our own
// exception throwing semantics.  Don't try this at home!

MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_bad_exception(
    void) {
  mozalloc_abort("fatal: STL threw bad_exception");
}

[...]

MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_out_of_range(
    const char* msg) {
  mozalloc_abort(msg);
}

[ldionne]

In D118733#3307739, @dim wrote:

FWIW, this breaks compilation of Firefox:
[...]

Thanks for the heads up. I just sent a courtesy email to a Firefox contributor to make them aware of this problem and CC'd you on the thread. Long story short, they are violating the Standard and there's nothing we can do about it -- we can only be nice and give them a heads up, which I did.

libcxx/include/string

 operator+(const basic_string<_CharT, _Traits, _Allocator>& __x, const _CharT* __y);
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __x, _CharT __y);
 
 _LIBCPP_EXTERN_TEMPLATE(_LIBCPP_FUNC_VIS string operator+<char, char_traits<char>, allocator<char> >(char const*, string const&))
 
-#ifndef _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS
-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;
-    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
-};
-#endif
-
 template <class _Iter>
 struct __string_is_trivial_iterator : public false_type {};
 
 template <class _Tp>
 struct __string_is_trivial_iterator<_Tp*>
     : public is_arithmetic<_Tp> {};
 
 template <class _Iter>

 #ifndef _LIBCPP_HAS_NO_CHAR8_T
     _LIBCPP_PREFERRED_NAME(u8string)
 #endif
 #ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
     _LIBCPP_PREFERRED_NAME(u16string)
     _LIBCPP_PREFERRED_NAME(u32string)
 #endif
     basic_string
-#ifndef _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS
-    : private __basic_string_common<true> // This base class is historical, but it needs to remain for ABI compatibility
-#endif
 {
 public:
     typedef basic_string                                 __self;
     typedef basic_string_view<_CharT, _Traits>           __self_view;
     typedef _Traits                                      traits_type;
     typedef _CharT                                       value_type;
     typedef _Allocator                                   allocator_type;
     typedef allocator_traits<allocator_type>             __alloc_traits;

libcxx/src/string.cpp

 #include "__debug"
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 #   include "cwchar"
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #ifndef _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS
-void __basic_string_common<true>::__throw_length_error() const {
+
+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 {
         _VSTD::__throw_length_error("basic_string");
     }
-
-void __basic_string_common<true>::__throw_out_of_range() const {
+    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const {
         _VSTD::__throw_out_of_range("basic_string");
     }
+};
+
 #endif
 
 #define _LIBCPP_EXTERN_TEMPLATE_DEFINE(...) template __VA_ARGS__;
 #ifdef _LIBCPP_ABI_STRING_OPTIMIZED_EXTERNAL_INSTANTIATION
     _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, char)
 #   ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
         _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE_DEFINE, wchar_t)
 #   endif