Skip to content
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.

Sign up for GitHub

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

Jump to bottom

poisoned_hash_helper.h relies on forward-declaring std::hash #56938

Closed
philnik777 opened this issue Aug 4, 2022 · 0 comments · Fixed by #108296
Closed

poisoned_hash_helper.h relies on forward-declaring std::hash #56938

philnik777 opened this issue Aug 4, 2022 · 0 comments · Fixed by #108296
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. test-suite

Comments

@philnik777
Copy link
Contributor

poisoned_hash_helper.h currently errors when adding #include <functional> and relies on a forward-declaration from <type_traits>.
@philnik777 philnik777 added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 4, 2022
ldionne added a commit to ldionne/llvm-project that referenced this issue Sep 11, 2024
@ldionne
[libc++][modules] Refactor poisoned_hash_helper
c5a811d 
The poisoned_hash_helper header was relying on an implicit forward
declaration of std::hash located in <type_traits>. When we improve
the modularization of the library, that causes issues, in addition
to being a fundamentally non-portable assumption in the test suite.

It turns out that the reason for relying on a forward declaration
is to be able to test that std::hash is *not* provided if we don't
include any header that provides it. But testing that is actually
both non-portable and not really useful.

Indeed, what harm does it make if additional headers provide std::hash
specializations? That would certainly be conforming -- the Standard
never requires an implementation to avoid providing a declaration when
a given header is included, instead it mandates what *must* be provided
for sure. In that spirit, it would be conforming for e.g. `<cstddef>`
to define the hash specializations if that was our desire. I also don't
read https://wg21.link/P0513R0 as going against that statement. Hence,
this patch just removes that test which doesn't carry its weight.

Fixes llvm#56938
@ldionne ldionne mentioned this issue Sep 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. test-suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[libc++][modules] Refactor poisoned_hash_helper ldionne/llvm-project
1 participant
@philnik777