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.
Details
- Reviewers
ldionne • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG065ac30026d5: [libc++] LWG3001: add `remove_extent_t` to `weak_ptr::element_type`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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. |
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. |
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. |
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). |
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.
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. |
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 |
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 >=. | |
510 | Pre-existing nit would be nice to fix: type= should be type = |
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:
First off, I agree with your proposal for this patch (which is "revert to > and take this discussion about conventions to a separate PR"). # 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? |
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. |
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. |
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).
clang-format not found in user’s local PATH; not linting file.