[libcxx-commits] [PATCH] D112092: [libc++] LWG3001: add `remove_extent_t` to `weak_ptr::element_type`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 5 inline comments as done.
var-const added inline comments.


================
Comment at: libcxx/include/__memory/shared_ptr.h:378
 struct __compatible_with
-#if _LIBCPP_STD_VER > 14
+#if _LIBCPP_STD_VER >= 17
     : is_convertible<remove_extent_t<_Tp>*, remove_extent_t<_Up>*> {};
----------------
Mordante wrote:
> Quuxplusone wrote:
> > Quuxplusone wrote:
> > > 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).
> > @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.)
> 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`.
(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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112092/new/

https://reviews.llvm.org/D112092