This is an archive of the discontinued LLVM Phabricator instance.

Remove `_VSTD`
AbandonedPublic

Authored by philnik on Dec 10 2018, 9:18 AM.

Details

Reviewers
mclow.lists
ldionne
jdoerfert
EricWF
Group Reviewers
Restricted Project
Summary

Quick Question: What's the difference between writing _VSTD:: and std::? Nothing.

OK... technically _VSTD:: expands to std::__version (where __version is the current ABI namespace). But this makes no functional difference. This lack of functional difference makes _VSTD:: confusing, and makes it unclear when it should be used.

Readers may ask why are some calls in libc++ using std:: while others are using _VSTD::. And contributors are often confused about when _VSTD is needed, resulting in surperflous usages (which further cause confusion).

The effect of std::, however, is perfectly clear to all readers of C++.

This patch replaces existing usages of _VSTD with std.

So if std:: is simpler and clearer, why do we have _VSTD to begin with? The macro has been present since the beginning, while libc++'s ABI versioning scheme was still being designed. It was envisioned to support cases where two versioning namespaces were present at the same time. But this is never the case, and it's now clear that the theoretical version mixing would never work, even with _VSTD. In summary _VSTD was preemptively introduced as a solution without a problem.

Diff Detail

Event Timeline

EricWF created this revision.Dec 10 2018, 9:18 AM
jfb added a comment.Dec 10 2018, 9:24 AM

Quick Question: What's the difference between writing _VSTD:: and std::? Nothing.

I thought it was std:: _LIBCPP_ABI_NAMESPACE ?

(Hm, i thougth replying by mail used to work)

Not hardcoding "::std" simplifies changing that namespace for whatever reasons.
I'm not sure if it is supported or not, but at least one person in
#llvm asked about that/did that.

ldionne accepted this revision.Dec 10 2018, 10:43 AM
In D55517#1325586, @jfb wrote:

Quick Question: What's the difference between writing _VSTD:: and std::? Nothing.

I thought it was std:: _LIBCPP_ABI_NAMESPACE ?

That's correct, however I don't think it's a big deal. Indeed, discussed it and we couldn't find a reason why we would ever want to call a function that is not in the current ABI's namespace. And even if we did, we could always use std::<ABI namespace>::function.

Not hardcoding "::std" simplifies changing that namespace for whatever reasons.
I'm not sure if it is supported or not, but at least one person in
#llvm asked about that/did that.

It's already impossible to just rename namespace std because some places hardcode things like namespace std { ... } (specifically, places where we purposefully don't use the ABI versioning namespace use this pattern). I don't think it's a good idea to refrain from making this change "just in case". I'm not saying it will never be useful, but let's revisit this decision when/if we get an actual use case for changing the namespace name? And at that point we could address the situation fully by also handling places that explicitly say namespace std { ... }.

I personally like this change but I'd wait a bit to see whether other people oppose (and why) before submitting.

This revision is now accepted and ready to land.Dec 10 2018, 10:43 AM
jfb added a comment.Dec 10 2018, 10:47 AM
In D55517#1325586, @jfb wrote:

Quick Question: What's the difference between writing _VSTD:: and std::? Nothing.

I thought it was std:: _LIBCPP_ABI_NAMESPACE ?

That's correct, however I don't think it's a big deal. Indeed, discussed it and we couldn't find a reason why we would ever want to call a function that is not in the current ABI's namespace. And even if we did, we could always use std::<ABI namespace>::function.

Sure, my point is that the commit message isn't right, and should be updated to have this rationale.

In D55517#1325586, @jfb wrote:

Quick Question: What's the difference between writing _VSTD:: and std::? Nothing.

I thought it was std:: _LIBCPP_ABI_NAMESPACE ?

It is. But std:: and std::__1 always resolve the same when used within namespace std::. There is no functional difference, which is confusing.

I personally like this change but I'd wait a bit to see whether other people oppose (and why) before submitting.

Thanks Louis!

mclow.lists added a comment.EditedDec 10 2018, 10:51 AM

I personally like this change but I'd wait a bit to see whether other people oppose (and why) before submitting.

I do not like this change.
I think that the loss of flexibility may come back to bite us in the future.

However, my main objection is that _VSTD is a noticeable chuck of all caps in the source code; it's an indication that we're deliberately qualifying something to disable ADL.
std is much less "in your face" - i.e, easier to miss.

I personally like this change but I'd wait a bit to see whether other people oppose (and why) before submitting.

I do not like this change.
I think that the loss of flexibility may come back to bite us in the future.

I would need to see a concrete example of the currently opaque "functionality" you're imagining before I can give this argument any credence.

However, my main objection is that _VSTD is a noticeable chuck of all caps in the source code; it's an indication that we're deliberately qualifying something to disable ADL.
std is much less "in your face" - i.e, easier to miss.

This should be enforced using tooling then.

Also, I'm less worried about being able to spot qualified calls (they're almost certainly correctly qualified). Seeing incorrectly unqualified calls is of more value.

In D55517#1325835, @jfb wrote:
In D55517#1325586, @jfb wrote:

Quick Question: What's the difference between writing _VSTD:: and std::? Nothing.

I thought it was std:: _LIBCPP_ABI_NAMESPACE ?

That's correct, however I don't think it's a big deal. Indeed, discussed it and we couldn't find a reason why we would ever want to call a function that is not in the current ABI's namespace. And even if we did, we could always use std::<ABI namespace>::function.

Sure, my point is that the commit message isn't right, and should be updated to have this rationale.

I think the commit message is correct. There is no functional difference, and the fact that _VSTD spells of std::__1 is functionally a distinction without a difference.
I purposefully omitted any mention of mixing ABI versions internally because the idea is non-functional. We only ever have one versioning namespace, what other version are we expecting to want to call?

jfb added a comment.Dec 10 2018, 11:27 AM
In D55517#1325835, @jfb wrote:
In D55517#1325586, @jfb wrote:

Quick Question: What's the difference between writing _VSTD:: and std::? Nothing.

I thought it was std:: _LIBCPP_ABI_NAMESPACE ?

That's correct, however I don't think it's a big deal. Indeed, discussed it and we couldn't find a reason why we would ever want to call a function that is not in the current ABI's namespace. And even if we did, we could always use std::<ABI namespace>::function.

Sure, my point is that the commit message isn't right, and should be updated to have this rationale.

I think the commit message is correct. There is no functional difference, and the fact that _VSTD spells of std::__1 is functionally a distinction without a difference.
I purposefully omitted any mention of mixing ABI versions internally because the idea is non-functional. We only ever have one versioning namespace, what other version are we expecting to want to call?

I think your commit message is fun and terse, but it doesn't say why you're actually correct. You're explaining it here, Marshall has voiced concerns about downsides. I think your commit message should explain this and say why you think the downsides aren't relevant. That makes it easier to go back to your change in the future and understand why the change was OK without looking at this discussion.

We have two different namespaces in libc++.

We have std, which is opened by namespace std {, and referred to using std::.
We have std::__1, which is opened by _LIBCPP_BEGIN_NAMESPACE_STD, and is referred to using _VSTD.
Some things live in former, and some (most) in the latter.

They're different, and the fact that std:: will find things in std::__1 doesn't change that.

EricWF edited the summary of this revision. (Show Details)Dec 10 2018, 11:49 AM

We have two different namespaces in libc++.

We have std, which is opened by namespace std {, and referred to using std::.
We have std::__1, which is opened by _LIBCPP_BEGIN_NAMESPACE_STD, and is referred to using _VSTD.
Some things live in former, and some (most) in the latter.

They're different, and the fact that std:: will find things in std::__1 doesn't change that.

Yes. We have versioned and unversioned components.

_VSTD can refer only to versioned components, while std:: is correct in all cases.

In D55517#1325917, @jfb wrote:

I think your commit message is fun and terse, but it doesn't say why you're actually correct. You're explaining it here, Marshall has voiced concerns about downsides. I think your commit message should explain this and say why you think the downsides aren't relevant. That makes it easier to go back to your change in the future and understand why the change was OK without looking at this discussion.

How's the new commit message?

We have two different namespaces in libc++.

We have std, which is opened by namespace std {, and referred to using std::.
We have std::__1, which is opened by _LIBCPP_BEGIN_NAMESPACE_STD, and is referred to using _VSTD.
Some things live in former, and some (most) in the latter.

They're different, and the fact that std:: will find things in std::__1 doesn't change that.

I agree they're technically different namespaces. However, I can't think of a situation where we want to refer to a function in something else than <the-namespace-of-the-current-ABI-version-or-the-unversionned-namespace>.

Also, I'm not sure code up to now has been written with that subtlety in mind, and as a result it's unclear to me whether doing any change that relies on _VSTD being used in precisely that way would work. In other words, it's quite possible that some uses of _VSTD:: right now should actually be using std::, and some uses of std:: should be using _VSTD::, so making a distinction between the two in the future might not work as intended. If we want to do this, I believe we should survey existing uses of std:: and _VSTD:: to make sure they're as expected.

jfb added a comment.Dec 10 2018, 12:16 PM
In D55517#1325917, @jfb wrote:

I think your commit message is fun and terse, but it doesn't say why you're actually correct. You're explaining it here, Marshall has voiced concerns about downsides. I think your commit message should explain this and say why you think the downsides aren't relevant. That makes it easier to go back to your change in the future and understand why the change was OK without looking at this discussion.

How's the new commit message?

Looks great, ty!

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

EricWF updated this revision to Diff 177616.Dec 10 2018, 4:28 PM

Merge with upstream.

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).

Thanks for the input. Indeed this was never a supported or intended use case.

I don't fully grok exactly what your need and how you're hacking up libc++ to meet it
(And I'm surprised it has managed to get you this far).

libc++ internals are just that: internal. Any external usage of the is unsupported,
and libc++ should make no attempt to support people who use and abuse them. The design
and maintenance of libc++ cannot be held at the behest of such users.

However, libc++ is receptive to the needs of vendors and your case shouldn't be any different.

First we need to know why use case exists, the details of it, and the rational behind it.
Then we can design and document a solution that works for both libc++ and the
vendor; one that is maintainable and testable.

I understand you're usage of libc++ is in it's infancy, and I would like to help you grow into
full fledged contributors. I suggest mailing a design document to the libc++-dev mailing lists
which explains your use case in detail. Only then can we start work on a solution.

EricWF planned changes to this revision.Dec 11 2018, 6:29 AM

I'm putting this change on hold until we can reasonable address Olivier's needs.

miyuki added a subscriber: miyuki.Jan 4 2019, 4:13 AM
ldionne added a reviewer: Restricted Project.Nov 2 2020, 3:46 PM
philnik commandeered this revision.Feb 8 2022, 6:35 AM
philnik added a reviewer: EricWF.
philnik added a subscriber: philnik.

This has been superseded by D117811.

philnik abandoned this revision.Feb 8 2022, 6:35 AM