[libc++] Move <__sso_allocator> out of include/ into src/. NFCI.

[• Quuxplusone]

Summary

This allocator is not intended for libc++'s users to use;
it's strictly an implementation detail of `src/locale.cpp`.
So, move it to the `src/include/` directory.

Drive-by const-qualify its comparison operators.

For consistency with `__hidden_allocator` (defined in `src/thread.cpp`),
do *not* remove it from "libcxx/lib/libc++unexp.exp",
"libcxx/utils/symcheck-blacklists/linux_blacklist.txt", etc.

[curdeius]

LGTM.

[ldionne]

So actually, at some point I just wanted to get rid of the src/include directory for simplicity and move everything to include/. I fully understand that this means we're shipping those headers even if they are technically not needed by end users. But is that really a problem? Concretely, the size increase in the distribution is in the noise (we might as well get rid of our long synopses). Thoughts?

[• Quuxplusone]

libcxx/src/locale.cpp

38

@ldionne wrote:

actually, at some point I just wanted to get rid of the src/include directory for simplicity and move everything to include/. I fully understand that this means we're shipping those headers even if they are technically not needed by end users. But is that really a problem?

I would very much prefer to have the directory structure match the intended usage. If we don't intend for end-users to include these headers, then they should not be permitted to include them (i.e., we should not ship them in the libc++ include path). Status quo is that the include/ directory is for libc++ headers, not for arbitrary other headers that happen to be required by the build process of arbitrary llvm-project artifacts (even if that llvm-project artifact is as near a miss as "the libc++.so library itself").

The main reason I want this file out of the include/ directory is so that the code's structure will match the code's intent. This is not a libc++ header; we don't want it shipped or installed; we don't want any other include/ header to ever depend on it; we don't want it to show up as a root in the dependency graph; we don't want it to undergo any of the things that normally happen to code in the include/ directory. So it shouldn't be there.

(That said, I would also be totally OK if you asked to revise this PR to eliminate the .h file entirely; its code could be placed directly into src/locale.cpp. That would be the code-review equivalent of the Google v. Oracle "fair use" decision ;) — it would be quite defensible, while deliberately omitting to either endorse the status-quo "Arthur's" way or set a competing precedent for your proposed "other" way.)

The idea of "have a defined public API / don't ship what your users can't use" is just generally a core belief of mine. But also, it reminds me of when uthash went through a similar process in https://github.com/troydhanson/uthash/pull/117 — having a bunch of "public but please-don't-use-this-it's-not-for-you" source code was a huge headache for the people making distributions. In libc++'s case obviously the problem is not as problematic and the solution is not as drastic; but still, we have a status-quo solution; we should use it.

[ldionne]

libcxx/src/locale.cpp

38

There are many headers that we don't want users including by themselves - basically anything that isn't a public header described in the standard library. We "enforce" that through naming. The benefit to putting all includes under include/ is that it's a simple model and it's what most libraries I've seen do.

I think our source of disagreement is that you value the "cleanliness" of minimizing what we ship/expose to users beyond the simplicity of the library itself, whereas for me it's the other way around.

Also, while eradicating the separate file altogether is clever to avoid creating a precedent, I don't think it would improve the code, so no.

[• Quuxplusone]

libcxx/src/locale.cpp

38

I think our source of disagreement is that you value the "cleanliness" of minimizing what we ship/expose to users beyond the simplicity of the library itself, whereas for me it's the other way around.

Not exactly. I mean, in this case, __sso_allocator is not a libc++ header. It's a piece of the src/ code; it belongs with the rest of src/. You wouldn't agitate for moving src/debug.cpp out of src/ into include/, right? So there's some distinction we're both making between "source code of the shared object" and "public-facing headers of libc++ itself". I'm just including __sso_allocator in the former set, rather than the latter set.

And while this is partly about how "we don't want it shipped or installed," it's also about how "we don't want any other include/ header to ever depend on it; we don't want it to show up as a root in the dependency graph — we don't want it to undergo any of the things that normally happen to [files] in the include/ directory." Right now it's hitting all of those things, and we actually want it to hit none of them.

[ldionne]

libcxx/src/CMakeLists.txt

67

Why is that needed? We normally don't add headers to the list of sources?

libcxx/src/locale.cpp

38

OK. I'm not swayed by your arguments, but let's do it, it'll make the distribution smaller.

Can you rename it to sso_allocator.h though, without tying it to locale too specifically?

[• Quuxplusone]

Rename the header file.
Also preemptively remove all non-.cpp files from LIBCPP_SOURCES to see what breaks. I imagine something will break.

[• Quuxplusone]

libcxx/src/CMakeLists.txt

67

I agree that AFAIK from my knowledge of CMake, it should not be needed. However, look at lines 19-21, 32-43, 89-90, 101-104.
I'd be happy to kill off those lines at the same time as this one (in fact I'll update this patch to do so), but if we keep those lines then I think we should keep this one too, because presumably they're all there for the same reason (or non-reason).

I see the comment Add all the headers to the project for IDEs on line 118. Is that the rationale for adding these .h files to LIBCXX_SOURCES?

libcxx/src/locale.cpp

38

Will do. I had put locale_ in there precisely because it's used only by locale and I assume the intent was to have it for that one purpose and not "leak" out any more widely than that; but I don't particularly care either way.

[Mordante]

libcxx/src/CMakeLists.txt

67

CMake indeed only needs for the IDE. (Unless the header is generated, but that's not the case here.)

[• Quuxplusone]

libcxx/src/CMakeLists.txt

67

@Mordante: "CMake indeed only needs for the IDE" — So, would you say we should keep all the files in CMakeLists.txt for the benefit of IDE-users? Your choice of the word "...only..." is throwing me a little bit. :)

I.e. is this part of the current patch (removing 20 lines from this file) a good idea or a bad idea? (I'm conservatively leaning toward "bad idea".)

[Mordante]

libcxx/src/CMakeLists.txt

67

As for the "only". CMake doesn't need the header to figure out the dependencies of the .cpp files, but CMake can't determine which headers should be part of your IDE project. You don't want every header (indirectly) included to be part of your project.

Having the headers in the list doesn't hurt us. So I think removing them is a bad idea, it hurts users using an IDE.

[• Quuxplusone]

Restore .h files to CMakeLists.txt, for the benefit of IDEs.
Drive-by const-qualify its comparison operators (since one of my greps turned up these lines and I noticed they were subpar).
Update summary to match my intended commit message.

libcxx/include/CMakeLists.txt

set(files
   __node_handle
   __nullptr
   __ranges/access.h
   __ranges/concepts.h
   __ranges/enable_borrowed_range.h
   __ranges/view.h
   __ranges/size.h
   __split_buffer
-  __sso_allocator
   __std_stream
   __string
   __threading_support
   __tree
   __tuple
   __undef_macros
   __utility/to_underlying.h
   algorithm

libcxx/include/module.modulemap

module std [system] {
   module __bit_reference { header "__bit_reference" export * }
   module __debug { header "__debug" export * }
   module __errc { header "__errc" export * }
   module __functional_base { header "__functional_base" export * }
   module __hash_table { header "__hash_table" export * }
   module __locale { header "__locale" export * }
   module __mutex_base { header "__mutex_base" export * }
   module __split_buffer { header "__split_buffer" export * }
-  module __sso_allocator { header "__sso_allocator" export * }
   module __std_stream { header "__std_stream" export * }
   module __string { header "__string" export * }
   module __tree { header "__tree" export * }
   module __tuple { header "__tuple" export * }
   module __undef_macros { header "__undef_macros" export * }
   module __node_handle { header "__node_handle" export * }
 
   module experimental {

libcxx/src/CMakeLists.txt

 if (LIBCXX_ENABLE_RANDOM_DEVICE)
   list(APPEND LIBCXX_SOURCES
     random.cpp
     )
 endif()
 
 if (LIBCXX_ENABLE_LOCALIZATION)
   list(APPEND LIBCXX_SOURCES
+    include/sso_allocator.h
     ios.cpp
     ios.instantiations.cpp
     iostream.cpp
     locale.cpp
     regex.cpp
     strstream.cpp
     )
 endif()

libcxx/src/include/sso_allocator.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef _LIBCPP___SSO_ALLOCATOR
-#define _LIBCPP___SSO_ALLOCATOR
+#ifndef _LIBCPP_SSO_ALLOCATOR_H
+#define _LIBCPP_SSO_ALLOCATOR_H
 
 #include <__config>
 #include <memory>
 #include <new>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
_LIBCPP_INLINE_VISIBILITY void deallocate(pointer __p, size_type __n)
         if (__p == (pointer)&buf_)
             __allocated_ = false;
         else
             allocator<_Tp>().deallocate(__p, __n);
     }
     _LIBCPP_INLINE_VISIBILITY size_type max_size() const throw() {return size_type(~0) / sizeof(_Tp);}
 
     _LIBCPP_INLINE_VISIBILITY
-    bool operator==(__sso_allocator& __a) const {return &buf_ == &__a.buf_;}
+    bool operator==(const __sso_allocator& __a) const {return &buf_ == &__a.buf_;}
     _LIBCPP_INLINE_VISIBILITY
-    bool operator!=(__sso_allocator& __a) const {return &buf_ != &__a.buf_;}
+    bool operator!=(const __sso_allocator& __a) const {return &buf_ != &__a.buf_;}
 };
 
 _LIBCPP_END_NAMESPACE_STD
 
-#endif // _LIBCPP___SSO_ALLOCATOR
+#endif // _LIBCPP_SSO_ALLOCATOR_H

libcxx/src/locale.cpp

 #  include "type_traits"
 #endif
 #include "clocale"
 #include "cstring"
 #if defined(_LIBCPP_MSVCRT)
 #define _CTYPE_DISABLE_MACROS
 #endif
 #include "cwctype"
-#include "__sso_allocator"
 #if defined(_LIBCPP_MSVCRT) || defined(__MINGW32__)
 #include "__support/win32/locale_win32.h"
 #elif !defined(__BIONIC__) && !defined(__NuttX__)
 #include <langinfo.h>
 #endif
 #include <stdlib.h>
 #include <stdio.h>
 #include "include/atomic_support.h"
+#include "include/sso_allocator.h"
 #include "__undef_macros"
 
 // On Linux, wint_t and wchar_t have different signed-ness, and this causes
 // lots of noise in the build log, but no bugs that I know of.
 #if defined(__clang__)
 #pragma clang diagnostic ignored "-Wsign-conversion"
 #endif