This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Throw correct exception from std::vector::reserve

Authored by miyuki on Oct 19 2021, 5:35 AM.



According to the standard [vector.capacity]/5, std::vector<T>::reserve
shall throw an exception of type std::length_error when the requested
capacity exceeds max_size().

This behavior is not implemented correctly: the function 'reserve'
simply propagates the exception from allocator<T>::allocate. Before
D110846 that exception used to be of type std::length_error (which is
correct for vector<T>::reserve, but incorrect for

This patch fixes the issue and adds regression tests.

Diff Detail

Event Timeline

miyuki requested review of this revision.Oct 19 2021, 5:35 AM
miyuki created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2021, 5:35 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Oct 19 2021, 11:12 AM

Do we need to fix this in vector<bool> too?

1 ↗(On Diff #380661)

I assume you thought about it and there's no way to test this without relying on libc++ internal details? It would be great if there was a way to avoid that.

33 ↗(On Diff #380661)

I don't even think this use of an internal detail is correct. This should be std::vector<bool, limited_allocator<bool, max_cap_words> > — the allocator's value_type should be the value_type of the container. vector<bool> will rebind the allocator internally as an implementation detail, but the user doesn't see that and in fact isn't allowed to pass in a wrongly-typed allocator AFAIK — I think this code is "library UB" right now.

I think the answer is to pass in a limited_allocator<bool, 10>, and then try to v.reserve(10 * 64 * 100), which "ought to be big enough for anybody," even some hypothetical system where vector<bool>::__storage_type is 128-bit or 256-bit or 6399-bit. (Premise: I don't think we need to test that the happy path on line 43 doesn't throw. We already test that in plenty of places, right? But actually, even if we wanted to test that, v.reserve(10) should still hit the happy path.)

miyuki updated this revision to Diff 380995.Oct 20 2021, 9:29 AM
  1. Addressed reviewers' comments.
  2. Added a prerequisite patch.
miyuki marked an inline comment as done.Oct 20 2021, 9:52 AM

LGTM now! (Of course now D112150 blocks this one: but hey, good, we fixed another bug too. :))



ldionne accepted this revision.Oct 20 2021, 2:10 PM
This revision is now accepted and ready to land.Oct 20 2021, 2:10 PM
This revision was automatically updated to reflect the committed changes.