Page MenuHomePhabricator

[libc++][ranges] Implement `uninitialized_value_construct{,_n}` and `uninitialized_fill{,_n}`.
ClosedPublic

Authored by var-const on Dec 13 2021, 4:03 AM.

Details

Summary

Also:

  • refactor out __voidify;
  • use the destroy algorithm internally;
  • refactor out helper classes used in tests for uninitialized_* algorithms.

Diff Detail

Event Timeline

var-const created this revision.Dec 13 2021, 4:03 AM
var-const requested review of this revision.Dec 13 2021, 4:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 4:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Dec 13 2021, 1:19 PM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/__memory/ranges_uninitialized_algorithms.h
49–51

How many columns do you have your Clang format configured to?

96

Please use namespace uninitialized_value_construct_ns and __fn until we've settled on whether we want to move everything to a different convention, to avoid inconsistencies.

libcxx/include/__memory/uninitialized_algorithms.h
100

Let's make those _LIBCPP_HIDE_FROM_ABI.

152

Here and elsewhere.

libcxx/include/__memory/voidify.h
26–30

Any reason why we don't literally copy-paste the definition from the Standard?

This revision now requires changes to proceed.Dec 13 2021, 1:19 PM
var-const marked 2 inline comments as done.Dec 14 2021, 4:59 PM
var-const added a subscriber: Quuxplusone.
var-const added inline comments.
libcxx/include/__memory/ranges_uninitialized_algorithms.h
49–51

I'm actually not using clang-format yet (though I do intend to start using it), so this is formatted manually. I set it to 100 columns (FWIW, my personal preference is 80, but a lot of code I've seen uses 100, and quite a few declarations are by necessity verbose, making me lean towards 100).

libcxx/include/__memory/uninitialized_algorithms.h
100

I marked them _LIBCPP_INLINE_VISIBILITY and inline for consistency with newer algorithms (e.g. uninitialized_move). Please let me know if either or both of these changes are wrong.

Regarding the visibility annotation -- I realize _LIBCPP_INLINE_VISIBILITY is just an "alias" for _LIBCPP_HIDE_FROM_ABI. Which form is preferable here? The existing algorithms use _LIBCPP_INLINE_VISIBILITY (when they use an annotation).

152

Thanks!

libcxx/include/__memory/voidify.h
26–30

This is based on a previous (unfinished) discussion in D115315. I think this form was advocated by @Quuxplusone. I didn't have a strong preference at the time of the discussion, but currently I'm leaning towards going with the Standard's definition.

My understanding is that the main advantage of using the C-style cast is its simplicity. However, it now seems to me that it's not so much simplicity as brevity, which isn't the same thing. The implementation from the Standard is, without doubt, clunky, however it a) explains exactly what's going on and b) draws attention to the fact that something unusual is going on. A C-style cast, however, can do one of several things, and remembering the rules and going through the exercise of applying them in this case is, I think, an extra burden for the reader; furthermore, given the C-style cast's brevity, the reader may be _unlikely_ to realize it's even an exercise worth going through because nothing really draws attention to it (other than "Why are we using a C-style cast?").

Finally, copy-pasting the Standard's definition is conceptually simpler. For the reader, there is no need to mentally verify that the two forms achieve exactly the same effect.

For these reasons, I'm currently leaning towards changing this to use the static_cast + const_cast combo. However, I would wait for @Quuxplusone's comments before making the change.

var-const updated this revision to Diff 394416.Dec 14 2021, 5:00 PM
var-const marked an inline comment as done.

Address feedback, rebase on main.

ldionne accepted this revision.Dec 15 2021, 11:10 AM

LGTM with my comments applied. Thanks!

libcxx/include/__memory/construct_at.h
47–49

We don't need this anymore since we are using __destroy from the is_array_v overload of destroy_at below.

libcxx/include/__memory/ranges_uninitialized_algorithms.h
49–51

Our libcxx/.clang-format uses 120 columns because indeed, we often have very verbose declarations.

libcxx/include/__memory/uninitialized_algorithms.h
100

_LIBCPP_HIDE_FROM_ABI is the newer name for _LIBCPP_INLINE_VISIBILITY. I added an alias instead of renaming everything to _LIBCPP_HIDE_FROM_ABI because that would have created a lot of code churn.

I would say use _LIBCPP_HIDE_FROM_ABI for all new functions and functions that you are touching anyway.

libcxx/include/__memory/voidify.h
26–30

I +1 going with the standard's definition (and that's what I did when I first introduced __voidify before we removed it).

libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.fill/ranges_uninitialized_fill.pass.cpp
99–100

Since we are not testing std::destroy here, I would *not* add these assertions (here and in the tests for uninitialized_default_construct). That will make it more obvious that we only need to destroy them as "boilerplate" for testing the rest, but we're not actually trying to check anything related to destroy itself.

Can you pull out that refactoring and the other ones in the .h file for uninitialized_default_construct in its own patch? It should be trivial to approve but it'll simplify this patch.

This revision is now accepted and ready to land.Dec 15 2021, 11:10 AM
Quuxplusone added inline comments.Dec 15 2021, 5:19 PM
libcxx/include/__memory/voidify.h
26–30

I'm okay with reintroducing __voidify and copy-pasting from the Standard.
(Personally I still think there was nothing wrong with (void*), but if you're waiting for my feedback here, I'm saying "do what Louis says.")

var-const updated this revision to Diff 394713.Dec 15 2021, 6:44 PM
var-const marked 2 inline comments as done.
  • address feedback;
  • run libcxx-generate-files;
  • rebase on main.
var-const updated this revision to Diff 394717.Dec 15 2021, 6:51 PM
var-const marked 5 inline comments as done.

Run clang-format on newly-added files.

libcxx/include/__memory/construct_at.h
47–49

I had to replicate the "is array" SFINAE in the internal versions of __destroy, and unfortunately the forward declaration is now once again necessary.

The new change also makes it so that deleting an array of arrays now works in C++17 (previously it only worked in C++20):

int x[][2] = {{0, 1}, {2, 3}, {4, 5}};
std::destroy(x, x + 3); // Previously worked in C++20 mode but not in C++17 mode
// Now works in both modes.

From the discussions I've seen on other patches, I don't think these functions should go out of their way to prevent usability improvements from later standards from seeping into older language modes (FWIW, in libstdc++ the code snippet works in C++17 mode). However, this is still something to call out, and please let me know if you think it's an issue.

libcxx/include/__memory/ranges_uninitialized_algorithms.h
49–51

(I personally prefer less columns because it makes it easier to see several files side-by-side)

Reformatted the files newly-added in this patch with .clang-format. I think I'll do an immediate follow-up once this patch lands to reformat ranges_uninitialized_algorithms with clang-format (to avoid introducing extra delta to this patch).

libcxx/include/__memory/uninitialized_algorithms.h
100

Done, thanks for the explanation.

libcxx/include/__memory/voidify.h
26–30

Thanks! Changed the implementation of __voidify (and added a comment to explain the purpose of the casts).

libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.fill/ranges_uninitialized_fill.pass.cpp
99–100

Removed from the code added in this patch, will create a separate patch for uninitialized_default_construct.

var-const marked an inline comment as done.Dec 15 2021, 6:52 PM
var-const updated this revision to Diff 394734.Dec 15 2021, 8:15 PM

Fix compilation

var-const updated this revision to Diff 395007.Dec 16 2021, 2:53 PM

Fix the CI (don't use C++14 and C++17 features in C++11 mode)

var-const updated this revision to Diff 395043.Dec 16 2021, 6:54 PM

Fix CI, fix and update the status page.

ldionne added inline comments.Dec 17 2021, 11:19 AM
libcxx/include/__memory/construct_at.h
47–49

We could restore the previous behavior by enclosing the array version of __destroy_at in #if _LIBCPP_STD_VER > 17.

I have a slight (but not strong) preference for that since it keeps us strictly conforming, and allows us to be lazy and not add a libc++ specific test for that "extension". I'll let you decide what to do here, but if you do support the extension, please add a test.

var-const updated this revision to Diff 395370.Dec 19 2021, 9:05 PM
var-const marked an inline comment as done.

Make destroy_at strictly conforming in C++17, rebase on main.