[libc++] Make _VSTD and alias for std

[philnik]

Summary

There is no practical difference between _VSTD and std so we should just remove _VSTD. This is the first step.

[dim]

How do you mean "no practical difference"? _VSTD is defined as std::_LIBCPP_ABI_NAMESPACE, and in turn, _LIBCPP_ABI_NAMESPACE is configurable by the user, or defaults to mangling the ABI version in it. So this would be a huge change, for unknown benefit?

[philnik]

In D117811#3259144, @dim wrote:

How do you mean "no practical difference"? _VSTD is defined as std::_LIBCPP_ABI_NAMESPACE, and in turn, _LIBCPP_ABI_NAMESPACE is configurable by the user, or defaults to mangling the ABI version in it. So this would be a huge change, for unknown benefit?

This patch isn't changing in which namespace the symbols are. Everything is still in std::_LIBCPP_ABI_NAMESPACE. _VSTD is only used when accessing anything in std::_LIBCPP_ABI_NAMESPACE and since _LIBCPP_ABI_NAMESPACE is always an inline namespace all the symbols are also accessible in std.

[philnik]

• Fix CI

[lebedev.ri]

How can there be no practical difference if one is a configurable 'macro' and other isn't?

In D55517#1326179, @__simt__ wrote:

Hello Eric, everyone else,

We have projects that are getting great mileage from this macro right now, we'd be sad to see it go. 😰

If you look at https://github.com/ogiroux/freestanding you'll get the gist of what we're doing with it: we don't just change the inline namespace name, we also change the std:: namespace name using the macro. This was never the intended use, I realize, but it allows us to make progress with e.g. side-by-side co-existence for a large & important subset. In other words, the presence of this macro helped make libc++ adaptable the way needed it to be, and may slowly transform us into contributors even (hence netting you great mileage too).

So our view is that we would not like to see this change made. Sorry.

Olivier

[philnik]

In D117811#3260819, @lebedev.ri wrote:

How can there be no practical difference if one is a configurable 'macro' and other isn't?

In D55517#1326179, @__simt__ wrote:

Hello Eric, everyone else,

We have projects that are getting great mileage from this macro right now, we'd be sad to see it go. 😰

If you look at https://github.com/ogiroux/freestanding you'll get the gist of what we're doing with it: we don't just change the inline namespace name, we also change the std:: namespace name using the macro. This was never the intended use, I realize, but it allows us to make progress with e.g. side-by-side co-existence for a large & important subset. In other words, the presence of this macro helped make libc++ adaptable the way needed it to be, and may slowly transform us into contributors even (hence netting you great mileage too).

So our view is that we would not like to see this change made. Sorry.

Olivier

_VSTD was never intended to be changed other than by changing _LIBCPP_ABI_NAMESPACE. The question I'm basically asking here is if there are projects that rely on the macro that don't seem to be abandoned and if there is a way around it for these projects. For example I think in the project linked it should be possible to change _LIBCPP_BEGIN_NAMESPACE_STD and using std in the headers should still work (https://godbolt.org/z/c7TTb1f9Y). Note that changing _LIBCPP_BEGIN_NAMESPACE_STD is just as unsupported as changing _VSTD.

[• Quuxplusone]

@philnik, I suggest opening a discussion thread on the Discord if you still want to pursue this direction at all.
D117811 duplicates D55517 (December 2018), which was abandoned, and I don't think we have any "new information" since then.
_VSTD and std currently are not synonymous — there are places you can grep for where we deliberately use std:: not _VSTD:: — and this seems like it would erase that deliberate difference. Whether the deliberate difference is useful, I don't know. But at the very least, you could propose a preliminary PR that removes just the difference (i.e. change just those few existing places to use _VSTD:: instead of std::), and let someone explain why even that would be a bad idea. It could be a learning experience for all of us. :)

[philnik]

In D117811#3261463, @Quuxplusone wrote:

@philnik, I suggest opening a discussion thread on the Discord if you still want to pursue this direction at all.
D117811 duplicates D55517 (December 2018), which was abandoned, and I don't think we have any "new information" since then.
_VSTD and std currently are not synonymous — there are places you can grep for where we deliberately use std:: not _VSTD:: — and this seems like it would erase that deliberate difference. Whether the deliberate difference is useful, I don't know. But at the very least, you could propose a preliminary PR that removes just the difference (i.e. change just those few existing places to use _VSTD:: instead of std::), and let someone explain why even that would be a bad idea. It could be a learning experience for all of us. :)

I am aware that this is a duplicate of D55517. I chose to create a new PR, since D55517 hasn't been updated in a few years. I agree that we don't have any new information, but I don't see any information in D55517 that would stop us from removing _VSTD.

[ldionne]

I think there is some misunderstanding around _VSTD::, _LIBCPP_ABI_NAMESPACE, and the notion that libc++ uses an unversioned namespace on purpose in a few places (notably libc++abi). Let me try to clear this up. First:

• _VSTD:: expands to std::<abi-namespace>, and can't be configured.
• _LIBCPP_ABI_NAMESPACE expands to <abi-namespace>, and is configurable.

However, because of inline namespaces, typing std::<abi-namespace>::foo and std::foo is strictly equivalent. Therefore, _VSTD::foo and std::foo are strictly equivalent for anything that was defined inside <abi-namespace>. There are some places, e.g. libc++abi, where we define types and functions inside namespace std { } without using <abi-namespace>. As far as I can tell, that was to be compatible with libstdc++ exceptions when we made the migration from libstdc++ to libc++ on Apple platforms ~10 years ago. Because these types and functions are not defined in std::<abi-namespace>, _VSTD cannot be used for qualifying them, because that would try to access them inside the <abi-namespace>. However, the other way around does work, i.e. using std:: in place of _VSTD::.

In particular, this patch does not propose removing the ability to customize the ABI namespace by changing _LIBCPP_ABI_NAMESPACE. That would still be supported, and if you customize <abi-namespace>, we'd still be using the correct namespace when doing std::foo, whether foo is a versioned symbol or not.

My main rationale for being in favour of this change is that we don't even use _VSTD:: consistently, and so any claims to be using _VSTD:: in fancy ways is guaranteed not to work without changes (and I suspect pretty major ones). In other words, I think we're cargo-culting the utility of this macro, and I would like to see it go away. I'm adding the libcxx vendors group to gather attention. If you can bring new information to the table (e.g. if you can show us reasonable uses of _VSTD that I am not aware of), then we can totally have this discussion, and we could even perhaps support that use case properly. However, I would really like to avoid making progress on this just based on fear and uncertainty.

However, my main concern is about breaking in-flight changes and downstream diffs by making such a wide ranging patch (I would have the same concern for e.g. clang-format on the whole code base at once). Instead, I would like that as a first step, we redefine _VSTD:: to being just std::, and start using std:: instead of _VSTD:: in new code we write. We could then also move from _VSTD:: to std:: in patches that e.g. modularize files or otherwise touch a lot of lines anyways. I'm accepting personally to show support for this. Let's give folks the opportunity to show concrete evidence of _VSTD needing to stay, and if that doesn't happen, we'll move forward.

[ldionne]

Just to clarify, the step 1 I'm requesting instead of this PR is to just change this:

#define _VSTD std

[__simt__]

I've mentioned this before, but NVIDIA defines _VSTD to something else in our product branch. This diff, here, implies that we will need a matching & opposite diff to further separate us from upstream.

I'm not necessarily saying you have to do something convenient for NVIDIA, but know how it's not ideal.

[ldionne]

In D117811#3261812, @__simt__ wrote:

I've mentioned this before, but NVIDIA defines _VSTD to something else in our product branch. This diff, here, implies that we will need a matching & opposite diff to further separate us from upstream.

I'm not necessarily saying you have to do something convenient for NVIDIA, but know how it's not ideal.

I would like to understand what your use case is so we can either support it properly, or not at all. The current situation where we're cargo culting the usefulness of _VSTD based on some vendors saying "don't make this change, it'll break us" is not good. I would like a concrete and actionable description of how you are using it so we can either decide "okay, that is a supported use case of libc++ and we're going to commit to supporting that properly", or to the contrary "this is twisted and we don't commit to supporting that".

[EricWF]

I fully support this change. I've wanted to kill _VSTD for a while. This will also reduce the memory usage of clang when compiling libc++ because it no longer needs to expand the macro and build nodes for the additional inline namespace specifier. Not sure how much this is worth, but it's something.

[philnik]

Only change _VSTD

[philnik]

The build failure looks to me like a gcc bug. Can anybody confirm this? Or is it implementation divergence?

[dim]

In D117811#3262289, @philnik wrote:

The build failure looks to me like a gcc bug. Can anybody confirm this? Or is it implementation divergence?

As far as I can see, it checked whether clang-format made any corrections, and it apparently did:

+ git-clang-format --binary /usr/bin/clang-format --diff --extensions ,h,hh,hpp,hxx,c,cc,cxx,cpp HEAD~1 -- libcxx/benchmarks libcxx/include libcxx/src libcxx/test libcxxabi/fuzz libcxxabi/include libcxxabi/src libcxxabi/test

+ tee /home/libcxx-builder/.buildkite-agent/builds/c70c66b3b3e9-1/llvm-project/libcxx-ci/build/check-format/clang-format.patch
...
+ grep -q '^--- a' /home/libcxx-builder/.buildkite-agent/builds/c70c66b3b3e9-1/llvm-project/libcxx-ci/build/check-format/clang-format.patch
🚨 Error: The command exited with status 1

E.g., the clang-format produced diff was non-empty? It seems that it wanted to indent some of the preprocessor statements...

[philnik]

@dim clang-format is currently not enforced. I mean the gcc build failure.

[wmaxey]

As @__simt__ has stated, we do make use of this macro, but are open to the idea of an alternative that does provide similar configurability.

This macro is used to differentiate between two namespaces depending on how the project is included/used. It would be possible for us to move away from some such a mechanism, but ideally rather than deleting it, I would like to see it be replaced with something that allows us to assume the namespace we need to search isn't necessarily std::

[philnik]

In D117811#3262965, @wmaxey wrote:

As @__simt__ has stated, we do make use of this macro, but are open to the idea of an alternative that does provide similar configurability.

This macro is used to differentiate between two namespaces depending on how the project is included/used. It would be possible for us to move away from some such a mechanism, but ideally rather than deleting it, I would like to see it be replaced with something that allows us to assume the namespace we need to search isn't necessarily std::

We know that people change it, but we don't know how it is used. Could you provide a specific example that uses _VSTD? If we don't know how _VSTD is used we can't determine how to officially support these use cases or see if it is necessary for these use-cases to even have _VSTD. Would it be possible to just change _LIBCPP_BEGIN_NAMESPACE_STD? I don't think we'll remove that. It would probably also be possible to make that configurable by the user more easily.

[Mordante]

libcxx/include/__config

824

Please add some information regarding this change to the release notes.

[philnik]

• Add gcc workarounds
• Add release note

[ldionne]

Gentle ping @__simt__ @wmaxey. I genuinely want to understand whether/how we can support your use case properly and avoid creating needless pain for you folks, but I also want us to stop pretending that this macro has any non-accidental use in its current form. I'll approve and ask to ship this on Feb 3rd if there is no answer.

libcxx/docs/ReleaseNotes.rst

122

[Mordante]

libcxx/include/__format/format_arg.h

141 ↗

(On Diff #404283)

Why is this required? Is this the Add gcc workarounds? This deviates from the previous assertion that there was no difference between using std:: and std::__1::. The requirement to sometimes use std::_LIBCPP_ABI_NAMESPACE instead of std:: feels error prone to me.

[philnik]

libcxx/include/__format/format_arg.h

141 ↗

(On Diff #404283)

Yes, these are the GCC workarounds. I think this is a GCC bug, but nobody has confirmed it yet, so I didn't open an issue. It seems GCC doesn't handle friends properly with inline namespaces.

[• Quuxplusone]

libcxx/include/__format/format_arg.h

141 ↗

(On Diff #404283)

I think it should work to just remove _VSTD:: from this line, because make_format_args is already in the same namespace as basic_format_arg, isn't it? (And ADL applies to calls, not friend declarations.) Thus:

template <class _Ctx, class... _Args>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT
friend __format_arg_store<_Ctx, _Args...> make_format_args(const _Args&...);

(Drive-by fixed indentation.) If this way works, which I believe it should, I'd very much prefer to do it (unless @Mordante is planning some future change that will explain why we don't want to do this even though it works).

Ditto below.

Also, if this works, please land this should-be-NFC change (and below) in their own separate commit. Let's have changing-the-#define _VSTD be a one-line commit, so it's really really trivial to revert if needed.

[Mordante]

Request changes to figure out which compiler is wrong.

libcxx/include/__format/format_arg.h

141 ↗

(On Diff #404283)

Please make sure it's a GCC bug and not a Clang bug, feel free to discuss it on Discord.
I'm not happy when it's a GCC bug and we need to add _LIBCPP_ABI_NAMESPACE. This is error prone and only caught by the CI. (At least I expect most of us using Clang as compiler.)

[Mordante]

libcxx/include/__format/format_arg.h

141 ↗

(On Diff #404283)

It seems our messages crossed @Quuxplusone, I'll have a look at removing the _VSTD from the friend declarations.

[Mordante]

libcxx/include/__format/format_arg.h

141 ↗

(On Diff #404283)

It works for me locally so created D118719. In that case we don't need to verify which compiler is right.

[philnik]

libcxx/include/__format/format_arg.h

141 ↗

(On Diff #404283)

Based on http://eel.is/c++draft/basic.namespace#namespace.def.general-6 I would say that this is a GCC bug. Name lookup is the same even if "there are declarations of that name in the enclosing namespace". I don't know if name lookup is done with friend declarations, but I guess it must be.

[ldionne]

It turns out that the release is being cut today instead of Feb 4th, like I thought. @philnik please update this and ping me, we'll ship it today.

If some vendors want to weigh in against this change, now's the time. Once this is shipped and we've cut LLVM 14, we'll start removing instances of _VSTD:: in new code and in code that we're touching anyway.

[philnik]

Rebased

[philnik]

• Update release note

[ldionne]

Ship it!

[wmaxey]

I appreciate getting some time to evaluate the impact. After some investigation we have no issue with this change.

In D117811#3284240, @ldionne wrote:

Gentle ping @__simt__ @wmaxey. I genuinely want to understand whether/how we can support your use case properly and avoid creating needless pain for you folks, but I also want us to stop pretending that this macro has any non-accidental use in its current form. I'll approve and ask to ship this on Feb 3rd if there is no answer.

libcxx/docs/ReleaseNotes.rst

- ``std::filesystem::path::iterator``, which (in our implementation) stashes
   so that it is a conforming input iterator in C++17 and a conforming
   ``std::bidirectional_iterator`` in C++20. Before this release, it had set its
   ``reference`` type to ``const path&`` and its ``iterator_category`` to
   ``bidirectional_iterator_tag``, making it a non-conforming bidirectional iterator.
   After this change, ``for`` loops of the form ``for (auto& c : path)`` must be rewritten
   as either ``for (auto&& c : path)`` or ``for (const auto& c : path)``.
   ``std::reverse_iterator<path::iterator>`` is no longer rejected.
 
 - Removed the nonstandard default constructor from ``std::chrono::month_weekday``.
   You must now explicitly initialize with a ``chrono::month`` and
   ``chrono::weekday_indexed`` instead of "meh, whenever".
 
 - C++20 requires that ``std::basic_string::reserve(n)`` never reduce the capacity
   of the string. (For that, use ``shrink_to_fit()``.) Prior to this release, libc++'s
   ``std::basic_string::reserve(n)`` could reduce capacity in C++17 and before, but
   not in C++20 and later. This caused ODR violations when mixing code compiled under
   different Standard modes. After this change, libc++'s ``std::basic_string::reserve(n)``
   never reduces capacity, even in C++17 and before.
   C++20 deprecates the zero-argument overload of ``std::basic_string::reserve()``,
   but specifically permits it to reduce capacity. To avoid breaking existing code
   assuming that ``std::basic_string::reserve()`` will shrink, libc++ maintains
   the behavior to shrink, even though that makes ``std::basic_string::reserve()`` not
   a synonym for ``std::basic_string::reserve(0)`` in any Standard mode anymore.
 
 - The ``<experimental/coroutine>`` header is deprecated, as is any
   use of coroutines without C++20. Use C++20's ``<coroutine>`` header
   instead. The ``<experimental/coroutine>`` header will be removed
   in LLVM 15.
 
+- ``_VSTD`` is now an alias for ``std`` instead of ``std::_LIBCPP_ABI_NAMESPACE``.
+
 ABI Changes
 -----------
 
 - The C++17 variable templates ``is_error_code_enum_v`` and
   ``is_error_condition_enum_v`` are now of type ``bool`` instead of ``size_t``.
 
 - The C++03 emulation type for ``std::nullptr_t`` has been removed in favor of
   using ``decltype(nullptr)`` in all standard modes. This is an ABI break for

libcxx/include/__config

 #endif
 
 // Just so we can migrate to the new macros gradually.
 #define _LIBCPP_INLINE_VISIBILITY _LIBCPP_HIDE_FROM_ABI
 
 // Inline namespaces are available in Clang/GCC/MSVC regardless of C++ dialect.
 #define _LIBCPP_BEGIN_NAMESPACE_STD namespace std { inline namespace _LIBCPP_ABI_NAMESPACE {
 #define _LIBCPP_END_NAMESPACE_STD  } }
-#define _VSTD std::_LIBCPP_ABI_NAMESPACE
+#define _VSTD std
 _LIBCPP_BEGIN_NAMESPACE_STD _LIBCPP_END_NAMESPACE_STD
 
-#if _LIBCPP_STD_VER >= 17
+#if _LIBCPP_STD_VER > 14
 #define _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM \
   _LIBCPP_BEGIN_NAMESPACE_STD inline namespace __fs { namespace filesystem {
 #else
 #define _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM \
   _LIBCPP_BEGIN_NAMESPACE_STD namespace __fs { namespace filesystem {
 #endif
 
 #define _LIBCPP_END_NAMESPACE_FILESYSTEM \