This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Bump _LIBCPP_STD_VER to the next expected C++ version
ClosedPublic

Authored by philnik on Sep 5 2022, 9:06 AM.

Details

Summary

We've decided to use _LIBCPP_STD_VER >= xy while discussing to change the constexpr macros, so let's finally bump the version macro to match that.

Diff Detail

Event Timeline

philnik created this revision.Sep 5 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 9:06 AM
philnik requested review of this revision.Sep 5 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 9:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision as: Mordante.Sep 5 2022, 9:32 AM

We've decided to use _LIBCPP_STD_VER >= xy while discussing to change the constexpr macros, so let's finally bump the version macro to match that.

I don't recall that was decided during that in that review. Can you share the link where that decision was made? (Or remove that part of the commit message.)

Regardless of the objection above I still think this change is nice to have. Based on the track record of the committee I feel we can expect the 3 year cycle. (Even when 26 becomes 27 it will be a simple search and replace.)

A few nits otherwise LGTM. I feel more developers should have a look so I leave the final approval to them.

libcxx/include/__config
50–51
51

Please do the same for the test macro including the comment.

philnik updated this revision to Diff 458056.Sep 5 2022, 3:23 PM
philnik marked 2 inline comments as done.
  • Address comments

We've decided to use _LIBCPP_STD_VER >= xy while discussing to change the constexpr macros, so let's finally bump the version macro to match that.

I don't recall that was decided during that in that review. Can you share the link where that decision was made? (Or remove that part of the commit message.)

I brought it up during the discussion on Discord and nobody objected, so I assumed everybody was OK with it. @avogelsgesang and @ldionne also explicitly asked about it there.

libcxx/include/__config
51

The test macro is defined as 99 currently, so we can already use TEST_STD_VER >= 23.

avogelsgesang accepted this revision.Sep 5 2022, 4:03 PM

LGTM!

Are you planning to also replace existing usages of, e.g. #if _LIBCPP_STD_VER > 17 by #if _LIBCPP_STD_VER >= 20?

ldionne accepted this revision.Sep 8 2022, 1:19 PM

Are you planning to also replace existing usages of, e.g. #if _LIBCPP_STD_VER > 17 by #if _LIBCPP_STD_VER >= 20?

I'm not sure that part is worth the churn. I'd welcome a patch, but depending on how crazy it ends up being, I think it might not be the best idea.

This revision is now accepted and ready to land.Sep 8 2022, 1:19 PM

We've decided to use _LIBCPP_STD_VER >= xy while discussing to change the constexpr macros, so let's finally bump the version macro to match that.

I don't recall that was decided during that in that review. Can you share the link where that decision was made? (Or remove that part of the commit message.)

I brought it up during the discussion on Discord and nobody objected, so I assumed everybody was OK with it. @avogelsgesang and @ldionne also explicitly asked about it there.

I don't feel "I assumed everybody was OK with it" sounds as a discision.
I don't mind seeing it in new code. But rather not see existing code in, areas with a lot of active development, changed. Or changes that affect patches under review.

mclow.lists added inline comments.
libcxx/include/__config
51

Until C++23 is released, I would recommend not changing these macros.

The test macro is defined differently than the one in __config in an attempt to make it harder for people to depend on numbers that are not guaranteed to be stable.

The code XXX_STD_VER > 20 will work for all standard versions later than C++20.

We've decided to use _LIBCPP_STD_VER >= xy while discussing to change the constexpr macros, so let's finally bump the version macro to match that.

I don't recall that was decided during that in that review. Can you share the link where that decision was made? (Or remove that part of the commit message.)

I brought it up during the discussion on Discord and nobody objected, so I assumed everybody was OK with it. @avogelsgesang and @ldionne also explicitly asked about it there.

I don't feel "I assumed everybody was OK with it" sounds as a discision.
I don't mind seeing it in new code. But rather not see existing code in, areas with a lot of active development, changed. Or changes that affect patches under review.

I do remember we had a discussion on Discord and several folks chimed in, but I don't remember exactly who did. My understanding after the discussion on Discord is that we were going to move forward with this approach. My understanding is also that I was the only one to oppose at the beginning.

We've decided to use _LIBCPP_STD_VER >= xy while discussing to change the constexpr macros, so let's finally bump the version macro to match that.

I don't recall that was decided during that in that review. Can you share the link where that decision was made? (Or remove that part of the commit message.)

I brought it up during the discussion on Discord and nobody objected, so I assumed everybody was OK with it. @avogelsgesang and @ldionne also explicitly asked about it there.

I don't feel "I assumed everybody was OK with it" sounds as a discision.
I don't mind seeing it in new code. But rather not see existing code in, areas with a lot of active development, changed. Or changes that affect patches under review.

I do remember we had a discussion on Discord and several folks chimed in, but I don't remember exactly who did. My understanding after the discussion on Discord is that we were going to move forward with this approach. My understanding is also that I was the only one to oppose at the beginning.

IMO then, for the future, we should make it clear whether we reached consensus and what the way forward is.
I would say a good way to do that is to update our libc++ developer documentation to reflect the new agreement.

AFAIK we only agreed to update the macro so we can use >= 23 but not require to use this new style. I really would like to have one style in one file and not to mix them in one file.

libcxx/include/__config
51

@mclow.lists For my curiosity can you tell why you don't recommend to change this macro?