This is an archive of the discontinued LLVM Phabricator instance.

[libc++] LWG3001: add `remove_extent_t` to `weak_ptr::element_type`.
ClosedPublic

Authored by var-const on Oct 19 2021, 12:59 PM.

Details

Summary

Also fix a few places in the shared_ptr implementation where
element_type was passed to the __is_compatible helper. This could
result in remove_extent being applied twice to the pointer's template
type (first by the definition of element_type and then by the helper),
potentially leading to somewhat less readable error messages for some
incorrect code.

Diff Detail

Event Timeline

var-const requested review of this revision.Oct 19 2021, 12:59 PM
var-const created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2021, 1:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const added inline comments.Oct 19 2021, 1:05 PM
libcxx/include/__memory/shared_ptr.h
384–410

This is pretty verbose, but it seemed important to ensure no changes in behavior for each language version. Please let me know if you have any suggestions on how to improve this.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.weak/types.pass.cpp
42

I copied this from a similar test for shared_ptr.

jloser added a subscriber: jloser.Oct 19 2021, 1:24 PM
jloser added inline comments.
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.weak/types.pass.cpp
33

Why not use ASSERT_SAME_TYPE to be consistent instead of mixed use of ASSERT_SAME_TYPE and std::is_same? BTW, you could use std::is_same_v since this block is C++17 code.

ldionne requested changes to this revision.Oct 19 2021, 2:11 PM

Thanks for looking into this! Have a few comments but it LGTM in essence, once we've addressed the LWG issue backporting. I think this should simplify the patch a bit.

libcxx/include/__memory/shared_ptr.h
384–410

Per offline review: Since this is a LWG issue we're fixing, we normally apply it in all standard modes. So you can fix weak_ptr in C++17 and above in-place.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/weak_ptr.pass.cpp
85

You'll want to change this to #if TEST_STD_VER > 14 per my comment about backporting LWG issues.

This revision now requires changes to proceed.Oct 19 2021, 2:11 PM
Quuxplusone added inline comments.
libcxx/include/__memory/shared_ptr.h
384–410

It would be much simpler to just treat LWG3001 as a DR against C++17. The issue description of LWG3001 makes it very clear that the intent was always to remove_extent_t in C++17; that diff just got lost in the editorial shuffle. I'm not saying "do it," but I'm at least saying "I hope @ldionne tells you to do it." ;)

(Personally I would be interested in a followup PR that DR'ed the remove_extent_t change back to C++11 just for simplicity's sake, but I suppose there'd be no real justification for that.)

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.weak/types.pass.cpp
42

In both places, I think it would be valuable to include test<int[2]>. We expect foo_ptr<int[][2]>::element_type to be int[2]. I could imagine someone accidentally flubbing it to int with a pretty simple typo (changing remove_extent into remove_all_extents).
If we have a similar test for unique_ptr<X>::element_type, this comment would apply there too.

var-const updated this revision to Diff 380831.Oct 19 2021, 5:20 PM

Address feedback:

  • apply the fix to C++17 as well, not just C++20;
  • add a test case for a multidimensional array;
  • use test helper macros where appropriate.
var-const marked 3 inline comments as done.Oct 19 2021, 5:28 PM

Thanks for the comments, everyone! PTAL.

libcxx/include/__memory/shared_ptr.h
384–410

Thanks! This is also exactly what @ldionne suggested.

I'm not sure if further backporting can be done without a proposal?

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.weak/types.pass.cpp
33

Thanks for pointing it out, it's mostly because I copied it from a different file and didn't think to update it. Changed to use the macro both here and in the shared_ptr test this is copied from.

42

Thanks for the suggestion. I added this check to the body of main (adding the check to test doesn't immediately work because the ASSERT_SAME_TYPE(typename std::weak_ptr<T>::element_type, T) check presumes the type is not an array, and it seemed easier to test this separately rather than find a workaround).

The unique_ptr test has a different structure, and I'd also prefer to keep this patch focused on shared/weak pointers.

var-const marked 3 inline comments as done.Oct 19 2021, 5:28 PM
ldionne accepted this revision.Oct 20 2021, 10:40 AM

Some minor nits, that should fix the tests. LGTM once CI is passing.

Thanks for looking into this!

libcxx/include/__memory/shared_ptr.h
384–410

Indeed, let's just DR to C++17. That's what we always do. There would be no precedent for DRing to C++11.

1354
libcxx/include/memory
529–531
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.weak/types.pass.cpp
14–16
This revision is now accepted and ready to land.Oct 20 2021, 10:40 AM

Actually apply this change to C++17

Quuxplusone accepted this revision.Oct 20 2021, 12:02 PM

LGTM at this point modulo all the >= 17 that need to be > 14, and that we should see buildkite passing successfully before landing this.

libcxx/include/__memory/shared_ptr.h
378

Please keep this as > 14. Even though _LIBCPP_STD_VER will never actually be set to "15", the intuition is that C++2b behaves more like 23 than like 20, C++1z (e.g. "C++15") behaves more like 17 than like 14, and so on. So we pretty consistently use _LIBCPP_STD_VER > x and TEST_STD_VER > x instead of >=.
Ditto line 422; ditto throughout (and also the tests, such as where Louis commented about it).

510

Pre-existing nit would be nice to fix: type= should be type =
(Also pre-existing: typename enable_if<x>::type could be updated to __enable_if_t<x>. But I recommend against fixing that in this PR.)

LGTM at this point modulo all the >= 17 that need to be > 14, and that we should see buildkite passing successfully before landing this.

I'll mention that I actually suggested doing that to @var-const to see if it would fly with the other reviewers. We've definitely been using VER > n as a convention in libc++ and the test suite, that's one of the few places where we're pretty consistent. However, I've always found it a bit weird and IMO it would be more natural to use >=. @Quuxplusone Would you (or someone else lurking on this review) oppose to making such a change? If so, why?

This is 100% orthogonal to the actual review -- I suggest @var-const just reverts to using > for getting this patch landed, and we can settle on this question separately. I'm just curious to see where that discussion leads us.

I see buildkite is green, but for the record, I'd like to see this PR updated with >-instead-of->= and wait for buildkite to pass on that version also, before landing.

libcxx/include/__memory/shared_ptr.h
378

@ldionne wrote:

We've definitely been using VER > n as a convention in libc++ and the test suite, that's one of the few places where we're pretty consistent. However, I've always found it a bit weird and IMO it would be more natural to use >=. @Quuxplusone Would you (or someone else lurking on this review) oppose to making such a change? If so, why?

First off, I agree with your proposal for this patch (which is "revert to > and take this discussion about conventions to a separate PR").
I also agree that it's often kind of inconvenient for the reader to mentally translate > 14 into "ah, this feature is new in C++17." I bet we can even find one or two historical bugs due to people getting that math wrong.
However, I believe my "intuitive explanation" above is pretty teachable. And any proposal to change the convention needs to come with a proposal of what to do about C++2b: Do you propose that we should start landing patches with >= 23 instead of > 20, and update the __config line that right now says

#    define _LIBCPP_STD_VER 21  // current year, or date of c++2b ratification

? Or do you propose that we do a massive mechanical updating of > 20 to >= 23 every three years? or what?
(Therefore I'm happy to stop talking about this here, and you can just ping me in the I-think-unlikely event that such a PR materializes.)

Mordante added inline comments.
libcxx/include/__memory/shared_ptr.h
378

I don't object against using >=, as long as we fix all occurrence in all of libc++ and not start mixing styles. Else it will lead to more confusion.

For the C++2b issue; I would prefer to then use C++23. The C++ committee has been using their 3 year release train for several iterations and they keep on schedule. If for some reason C++23 becomes C++24, it would be a trivial search for 23'and replace them with 24.

var-const updated this revision to Diff 381418.Oct 21 2021, 4:03 PM

Replace >= 17 with > 14.

var-const updated this revision to Diff 381419.Oct 21 2021, 4:04 PM

Fix preexisting nit

var-const marked 5 inline comments as done.Oct 21 2021, 4:23 PM
var-const added inline comments.
libcxx/include/__memory/shared_ptr.h
378

(I have reverted to using the > 14 form for this patch)

For what it's worth, it does seem to me like the current convention is slightly less readable -- while it's trivial in any _individual_ instance to mentally substitute 17 and above for > 14 (for example), it does seem to add some mental strain, however slight, when skimming through code.

I did some simple grepping (with queries like VER > 14 under libcxx/), and looking at the results, both forms are used quite frequently. The > form is somewhat more common:

>= 03: 0 occurrences
>= 11: 1245 occurences
>= 14: 210
>= 17: 85
>= 20: 20

> 03: 20
> 11: 475
> 14: 612
> 17: 733
> 20: 118

What's interesting is that the >= form is much more common for C++11, whereas > is more common for newer standards.

The idea to use the expected standard release date as a placeholder and update it if necessary seems like a reasonable approach to me, for what it's worth. The potential s/23/24 change, while it might be somewhat large in terms of delta, is mechanical and self-contained and shouldn't break any users (and, moreover, might not need to happen at all). The benefits, however, are having arguably slightly more readable code. Giving that the benefit is continuing while the cost is contained within a few one-time cleanups, it seems like a reasonable compromise.

ldionne accepted this revision.Oct 22 2021, 10:39 AM

The CI failures are caused by an unrelated change which should have been fixed now. Feel free to commit as-is (or rebase onto main and re-upload to trigger the CI again if you want to be super super safe).

var-const updated this revision to Diff 381649.Oct 22 2021, 1:42 PM

Rebase on main -- hopefully the CI will pass now.

var-const updated this revision to Diff 381815.Oct 24 2021, 6:30 PM

Rebase again, this time actually including the commit that fixes the tests.

The CI passes now -- this should be good to merge.