Skip to content

[libc++] Falsely rejected code for std::unique_ptr because __compressed_pair requires different types. #91266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
SainoNamkho opened this issue May 6, 2024 · 4 comments · Fixed by #76756
Assignees
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. rejects-valid

Comments

@SainoNamkho
Copy link

SainoNamkho commented May 6, 2024

As shown in https://godbolt.org/z/4K6q9bo3e, only libc++ refuses to compile the following code

#include <memory>
#include <any>

void f(std::any) {}

std::unique_ptr<decltype(f), decltype(f)*> p(f, f);

I've not found anywhere in the draft saying this should be rejected.

  1. Why does __compressed_pair require the two template arguments to be distinct and the _CanBeEmptyBase traits? If they make it unable to compress, then an uncompressed layout is automatically produced, with no need to be explicitly rejected. And the indices used by __compressed_pair_elem suggest that it handles same types correctly.
  2. If it is reasonable to keep the implementation of __compressed_pair, is it a good solution to change unique_ptr into use something like conditional_t<same_as<pointer, deleter_type>, __compressed_pair<<pointer, deleter_type>, ...>?
@github-actions github-actions bot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 6, 2024
@philnik777
Copy link
Contributor

As the diagnostic says, the current implementation isn't ABI compatible with an older implementation. You are correct that it's non-conforming to reject this. Given that AFAIK nobody complained about it yet, it's not exactly a pressing issue though, and nobody got actually broken by the ABI change. Re. (2), I'd really like to avoid allowing this now. #76756 gets rid of __compressed_pair entirely and I'd rather avoid having to extend the ABI compatibility code. It's already complex enough.

@SainoNamkho
Copy link
Author

As the diagnostic says, the current implementation isn't ABI compatible with an older implementation. You are correct that it's non-conforming to reject this. Given that AFAIK nobody complained about it yet, it's not exactly a pressing issue though, and nobody got actually broken by the ABI change. Re. (2), I'd really like to avoid allowing this now. #76756 gets rid of __compressed_pair entirely and I'd rather avoid having to extend the ABI compatibility code. It's already complex enough.

Thanks a lot, I myself feel glad about the ABI breaking patch, if the team decides the change is worthy.

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented May 7, 2024

See https://reviews.llvm.org/D27565.

@EricWF said that

Grr... OK so this is currently ABI breaking because it re-arranges the layout of __compressed_pair<T1, T2> when T2 is empty but T1 is not. I might be able to work around this with some metaprogramming.

But in this issue both T1 and T2 are pointers, which shouldn't be affected. Should we static_assert !(is_same<_T1, _T2>::value && is_empty<_T1>::value) instead?

@philnik777
Copy link
Contributor

Since it will change in the next release, I'd rather not touch it at all until then. I don't think this is an especially pressing issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. rejects-valid
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants