std::unordered_map and std::unordered_set

[Avi Drissman]

As per base/containers/README.md:

Note that this advice never suggests the use of std::unordered_map and std::unordered_set. These containers provides similar features to the Abseil flat hash containers but with worse performance. They should only be used if absolutely required for compatibility with third-party code.

Is this something we want to enforce? These "only [to] be used if absolutely required" classes have pretty wide usage.

Avi

[Sylvain Defresne]

I am in favor of enforcing our recommendations automatically. This is better than relying on all reviewers catching those usage, and pointing to the correct documentation.

Though: one thing that I noticed with a recent change is that by default absl::flat_hash_set::find() is not heterogeneous because the default hash is not considered transparent. This can introduce problem if the key is raw_ptr<T> and the code is calling find() with a dangling pointer (this caused failures in https://chromium-review.googlesource.com/c/chromium/src/+/7493299, and I had to specify a different hash).

-- Sylvain

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAZnHVZDvQPYrxCaGEOvYvVe3oj5OJ%3DLwxDhoQFK9YHoAA%40mail.gmail.com.

[John Admanski]

I think specifically in the case of the unordered_* containers you could have some kind of lint or presubmit that warns you against using them.

That said, I think in general it's not really feasible to try and enforce the recommendations of that document, as selecting the appropriate container for a task is a non-trivial thing and the document is more aiming to provide good defaults and advice rather than a set of hard rules. Even the paragraph in question here was really just intended to highlight that unless you have a very specific reason why you need those containers in particular, they're probably not a good choice. It wasn't really intended to rise to the level of a ban, if that was the intent then it really would be better placed in the style guide itself.

(I'm not a decider or authoritative on this, I'm just chiming in because I was the author of that particular change)

-- John

[Alex Cooper]

FWIW, the guidance *not* to use std::unordered_map and std::unordered_set was only added (silently, unless I missed an email) about a year ago by this CL. Here's that file from prior to that commit, which while it doesn't ban, does discourage but calls out exceptions where it's fine to use them.

For what it's worth, I feel like I only noticed the change to recommend the absl:: types because I went to double check my memory of something on the table, and I don't remember any broad announcement preferring this type or as like a change to the style/recommendation even on this list.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJauWCHBoJ4Ny65-ZYRgkzB7Zp0m6-6z5HyVK48hNHqhbcqVyg%40mail.gmail.com.

[Alexei Svitkine]

If we wanted a presubmit to flag uses of those, here's the place where we have similar checks:

https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;drc=7060849d7ef815b99002bc8548ec5027e8d478bd;l=495

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGOLbz1OyssG75s%2B0hLDaUjoq-PPMP5oFKNP6h39C1qeDpYSeQ%40mail.gmail.com.

[Victor Vianna]

Maybe increasing the scope of the discussion but: the same problem occurs with other containers mentioned in that doc.

"Chromium code should always use base::circular_deque or base::queue in preference to std::deque or std::queue due to memory usage and platform variation." (docs)

There are 213 usages of std::deque outside of third_party (cs) and 400 of std::queue (cs).

"std::stack is like std::queue in that it is a wrapper around an underlying container. The default container is std::deque so everything from the deque section applies. Chromium provides base/containers/stack.h which defines base::stack that should be used in preference to std::stack." (docs)

There are 68 usages of std::stack outside of third_party (cs).

None of the above seems enforced by presubmit, since I could easily upload this CL.

[Victor Vianna]

> That said, I think in general it's not really feasible to try and enforce the recommendations of that document, as selecting the appropriate container for a task is a non-trivial thing and the document is more aiming to provide good defaults and advice rather than a set of hard rules

We can make these presubmit warnings instead of errors. Should we start with that?

https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=133;drc=fbffe55c2d7d156ce726a5850381664e33c1232e