Page MenuHomePhabricator

[libc++] Disentangle std::pointer_safety
ClosedPublic

Authored by ldionne on Apr 13 2021, 1:47 PM.

Details

Summary

This patch gets rid of technical debt around std::pointer_safety which,
I claim, is entirely unnecessary. I don't think anybody has used
std::pointer_safety in actual code because we do not implement the
underlying garbage collection support. As such, I think it's entirely
fine to get rid of complex workarounds whose goals were to avoid breaking
the ABI back in 2017.

I'm putting this up both to get reviews and to discuss this proposal for
a breaking change. I think we should be comfortable with making these
tiny breaks if we are confident they won't hurt anyone, which I'm fairly
confident is the case here.

Diff Detail

Event Timeline

ldionne requested review of this revision.Apr 13 2021, 1:47 PM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 1:47 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added a subscriber: Quuxplusone.

libcxx/docs/ReleaseNotes.rst could use some line breaks, and say "scoped enum" rather than "enum class".
The comment // !C++03 should of course be worded more rigorously.
P2186 "Removing Garbage Collection Support" is relevant, and perhaps it would be appropriate to just wait until that paper's landed for C++2b and then rip out the whole thing? If anyone out there would object to wholesale-ripping-GC-out-of-all-modes, IMO it would be useful to bring that up now rather than later.

ldionne updated this revision to Diff 337432.Apr 14 2021, 6:17 AM

Fix tests in c++03 mode, address Arthur's comments

libcxx/docs/ReleaseNotes.rst could use some line breaks, and say "scoped enum" rather than "enum class".

Agreed and done.

The comment // !C++03 should of course be worded more rigorously.

I don't agree, what's unclear about it? You'd prefer !defined(_LIBCPP_CXX03_LANG)?

P2186 "Removing Garbage Collection Support" is relevant, and perhaps it would be appropriate to just wait until that paper's landed for C++2b and then rip out the whole thing? If anyone out there would object to wholesale-ripping-GC-out-of-all-modes, IMO it would be useful to bring that up now rather than later.

Hmm, thanks for bringing that paper up. The only question is whether we should rip it out in all standard modes or keep it in C++11/14/17/20 with an empty implementation. I'd be tempted to make the proposed (current) change now to remove tech debt (with no impact on anyone cause nobody cares), and then once P2186 lands, remove it from C++ > 20 only. That would maximize the conformance of our implementation.

Quuxplusone accepted this revision as: Quuxplusone.Apr 14 2021, 12:11 PM

once P2186 lands, remove it from C++ > 20 only. That would maximize the conformance of our implementation

FWIW, I suspect that by the time p2186 lands, we'll be comfortable removing it in all modes; but I agree we don't need to solve that now.
LGTM, ship it!

libcxx/include/__memory/pointer_safety.h
50

You'd prefer !defined(_LIBCPP_CXX03_LANG)?

Yes, comparing the incidence of git grep 'endif.*C++03' versus git grep 'endif.*_LIBCPP_CXX03_LANG', I think the latter is better.

cebowleratibm requested changes to this revision.Apr 16 2021, 1:28 PM
cebowleratibm added a subscriber: cebowleratibm.
cebowleratibm added inline comments.
libcxx/include/__memory/pointer_safety.h
31

I suggest using attribute ((deprecated)) here.

This revision now requires changes to proceed.Apr 16 2021, 1:28 PM
ldionne marked an inline comment as done.Mon, May 3, 11:33 AM
ldionne added inline comments.
libcxx/include/__memory/pointer_safety.h
31

I was going to do it, but then I saw that the Standard didn't actually deprecate it. It seems that we haven't bothered. I think we should mark it as deprecated when (if) the Standard makes it so.

ldionne accepted this revision as: Restricted Project.Mon, May 3, 11:33 AM
ldionne marked an inline comment as done.
This revision was not accepted when it landed; it landed in state Needs Revision.Mon, May 3, 11:34 AM
This revision was automatically updated to reflect the committed changes.

@vvereschaka We're wondering if this commit is causing breakage in the llvm-clang-win-x-armv7l buildbot (e.g. https://lab.llvm.org/buildbot/#/builders/60/builds/3205), would you able to inspect and test in a similar setup/configuration? There are 13 tests failing; the substitutes-in-compile-flags.sh.cpp case should be handled by D102048, but we don't know exactly what caused the 12 other failing ones under std/atomics.

@vvereschaka We're wondering if this commit is causing breakage in the llvm-clang-win-x-armv7l buildbot (e.g. https://lab.llvm.org/buildbot/#/builders/60/builds/3205), would you able to inspect and test in a similar setup/configuration? There are 13 tests failing; the substitutes-in-compile-flags.sh.cpp case should be handled by D102048, but we don't know exactly what caused the 12 other failing ones under std/atomics.

Sorry for the noise, it seems like those atomics tests are working again in a newer test run. Nevermind...