This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove the deprecated <ext/hash_set> and <ext/hash_map>.
AbandonedPublic

Authored by ldionne on Mar 2 2022, 9:33 AM.

Details

Reviewers
philnik
howard.hinnant
EricWF
pcc
Quuxplusone
Group Reviewers
Restricted Project
Restricted Project
Summary

These have been deprecated since the initial import in 2010.

They can't be added into the modulemap at all, because (1) they depend on the setting of macro __DEPRECATED so they must be textual, but (2) they include detail headers like ext/__hash and __algorithm/is_permutation.h that aren't accessible to user code (and thus aren't accessible to textually-included headers either).

Well, "not accessible" merely means "triggers -Wprivate-header," which could be worked around (see D118616), but my thesis is that working around it would be uglier, and just killing off these deprecated headers once and for all would be nicer, as long as we agree we can get away with it.

This is also an application of my mantra "If it isn't tested, it doesn't work." Notice the removed tests: they don't actually test much of anything. So we have no idea whether this code actually works.

Diff Detail

Event Timeline

Quuxplusone created this revision.Mar 2 2022, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 9:33 AM
Quuxplusone requested review of this revision.Mar 2 2022, 9:33 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 2 2022, 9:33 AM
philnik accepted this revision as: philnik.Mar 2 2022, 9:39 AM
philnik added a reviewer: Restricted Project.

I like this, but we should probably also ask #libc_vendors if they are OK with this change.

I attempted in Feb 2019 here: https://reviews.llvm.org/D57688.

I ended up dropping the effort after the discussion at https://discourse.llvm.org/t/removing-deprecated-ext-hash-set-ext-hash-map-and-ext-hash/51068.

FWIW, I still support this removal, but I'd like to get a better understanding of what's broken by it before moving forward.

ldionne requested changes to this revision.Mar 2 2022, 10:03 AM
This revision now requires changes to proceed.Mar 2 2022, 10:03 AM

I attempted in Feb 2019 here: https://reviews.llvm.org/D57688.

I ended up dropping the effort after the discussion at https://discourse.llvm.org/t/removing-deprecated-ext-hash-set-ext-hash-map-and-ext-hash/51068.

FWIW, I still support this removal, but I'd like to get a better understanding of what's broken by it before moving forward.

An internal search yielded > 1000 hits for <ext/hash_map> and <ext/hash_set> in mostly third party code. I'd prefer to avoid this busywork so a no from me.

Here's one possible alternative, which would make everyone happy (I think):

  1. Move <ext/hash_map> & friends to another directory OUTSIDE of libcxx/include.
  2. When a CMake setting like LIBCXX_INSTALL_LEGACY_HASH_MAP is enabled, install it as part of install-cxx-headers. By default, this is OFF, but vendors who want to keep it can turn it on.
  3. Remove everything else related to those headers from our code base (i.e. take most of this patch).

That way, vendors could retain the headers with minimal effort, but we'd still get the benefit of not having to care about them anymore.

The lack of benefit for removing them has sometimes been brought up as an argument for keeping them around. I've never pushed back on that, since it's pretty reasonable (why cause pain for no reason?). However, given that now this is causing us problems (with D118616), I think we have a good reason to act on those headers.

Thoughts?

ldionne commandeered this revision.Sep 5 2023, 12:59 PM
ldionne edited reviewers, added: Quuxplusone; removed: ldionne.

[Github PR transition cleanup]

Dropping this again. I think we should still do this eventually, but this patch needs so many changes at this point that it's not worth reviving.

ldionne abandoned this revision.Sep 5 2023, 12:59 PM
libcxx/test/libcxx/extensions/hash/specializations.pass.cpp