This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Don't include <version> everywhere
AbandonedPublic

Authored by philnik on Jan 21 2023, 8:05 AM.

Details

Reviewers
ldionne
Mordante
var-const
huixie90
Group Reviewers
Restricted Project
Summary

Define the version macros that are required per-header instead

Diff Detail

Event Timeline

philnik created this revision.Jan 21 2023, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2023, 8:05 AM
philnik requested review of this revision.Jan 21 2023, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2023, 8:05 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 491112.Jan 21 2023, 5:37 PM

Try to fix CI

philnik updated this revision to Diff 491115.Jan 21 2023, 6:44 PM

Try to fix CI

Can you please explain why this is done? What are the benefits for users/libc++ itself?

I'm not convinced this is an improvement:

  • most (all) toplevel headers have a generated part, which to me looks like a potential source of bugs.
  • this may give different behaviour for users who use FTM, but not include the proper header. (Obviously that is a bug in their code, but currently it works, after this change is may break of change the behaviour.)
  • since we don't want to break older language versions we now duplicate the macros.

This change should definitely be mentioned in the Release Notes.

This source for https://libcxx.llvm.org/DesignDocs/FeatureTestMacros.html should be updated too.

@EricWF can you tell why the original script took the approach to have one version header?

libcxx/include/__format/format_functions.h
14

This will break compilation.

libcxx/utils/generate_feature_test_macro_components.py
572

Can you add the __cpp_lib_containers_ranges macro?

Can you please explain why this is done? What are the benefits for users/libc++ itself?

The main benefit is avoiding the portability pitfall.

I'm not convinced this is an improvement:

  • most (all) toplevel headers have a generated part, which to me looks like a potential source of bugs.

What exactly would be the problem? It's not significantly different from having #include <__config> or #pragma GCC system_header everywhere.

  • this may give different behaviour for users who use FTM, but not include the proper header. (Obviously that is a bug in their code, but currently it works, after this change is may break of change the behaviour.)

This is also the case when switching implementations, which is a portability pitfall right now.

  • since we don't want to break older language versions we now duplicate the macros.

I'm not sure what you mean. The macros would be defined in multiple places regardless of keeping includes.

This change should definitely be mentioned in the Release Notes.

This source for https://libcxx.llvm.org/DesignDocs/FeatureTestMacros.html should be updated too.

@EricWF can you tell why the original script took the approach to have one version header?

Can you please explain why this is done? What are the benefits for users/libc++ itself?

The main benefit is avoiding the portability pitfall.

That's a pitfall we have with our current design, but instead we create a language version upgrade pitfall in libc++. This means users who don't care about portability may get caught in this trap. I much rather have the portability trap, than the upgrade trap. (Preferable I would have no traps, but that ship has sailed years ago.)

I'm not convinced this is an improvement:

  • most (all) toplevel headers have a generated part, which to me looks like a potential source of bugs.

What exactly would be the problem? It's not significantly different from having #include <__config> or #pragma GCC system_header everywhere.

The difference is I need to type these lines. Now the header needs a magic comment to include an auto-generated part. Some headers don't have the new magic, like ratio. So when there will be an update to ratio (for example https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2734r0.pdf) the update script won't work out-of-the-box. So we need additional instructions on how to use the update script cmake target.

  • this may give different behaviour for users who use FTM, but not include the proper header. (Obviously that is a bug in their code, but currently it works, after this change is may break of change the behaviour.)

This is also the case when switching implementations, which is a portability pitfall right now.

See my remark above.

  • since we don't want to break older language versions we now duplicate the macros.

I'm not sure what you mean. The macros would be defined in multiple places regardless of keeping includes.

Mainly that there are now two places that may define the FTM, depending on the language version. Of course they should be in sync, but I see no real benefit from having the generated code in two places.

Can you please explain why this is done? What are the benefits for users/libc++ itself?

The main benefit is avoiding the portability pitfall.

That's a pitfall we have with our current design, but instead we create a language version upgrade pitfall in libc++. This means users who don't care about portability may get caught in this trap. I much rather have the portability trap, than the upgrade trap. (Preferable I would have no traps, but that ship has sailed years ago.)

There is no reason to use FTMs if you use only a single standard library though. They are only useful if you want your code to be portable.

I'm not convinced this is an improvement:

  • most (all) toplevel headers have a generated part, which to me looks like a potential source of bugs.

What exactly would be the problem? It's not significantly different from having #include <__config> or #pragma GCC system_header everywhere.

The difference is I need to type these lines. Now the header needs a magic comment to include an auto-generated part. Some headers don't have the new magic, like ratio. So when there will be an update to ratio (for example https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2734r0.pdf) the update script won't work out-of-the-box. So we need additional instructions on how to use the update script cmake target.

The scripts tells you that they are missing when they are.

ldionne requested changes to this revision.Jan 26 2023, 9:00 AM

Even after discussing this during live review, I am not sure this is worth doing. I understand that the goal is to make us more strict w.r.t. to the standard and remove a potential portability trap for users, and I think that's an interesting goal to pursue. I'm usually pretty supportive of those changes. However, in this case, I am just not convinced that the benefit outweighs the cost of doing this. Concretely, I don't think this bites any user (please LMK if I'm wrong here), and the cost for us is to have additional duplication in our headers.

libcxx/include/atomic
1524

Can we get a separate patch for *just this change*? I think I remember there was a pretty tricky bug around this very definition, and I'd like to evaluate the change on its own. To be clear, I like using _LIBCPP_STD_VER more than the macro, but I'm not sure that'll work here, which is what I want to think about in that other review.

Edit: I just checked with D133377 and I think this is correct, but please extract into a separate review since this is effectively a NFC.

libcxx/include/scoped_allocator
734

This is not sufficiently visible IMO, the tag should be something like BEGIN-FEATURE-TEST-MACROS or something like that.

libcxx/include/version
148

Also to be moved to a different review, please!

libcxx/test/libcxx/transitive_includes/cxx11.csv
122

Can you please figure out why this is detected now and wasn't before? This change should not be needed.

libcxx/utils/generate_feature_test_macro_components.py
564

If we ever move forward with this patch, I think we'd also want some sort of test to check that including e.g. <vector> does not define FTMs from e.g. <atomic>. This is hard to achieve, though -- we'd need a full granularization and public headers would only include the private headers they need to provide the standard API, and nothing else. And inside the library, implementation-detail headers would never include any public API header. That may be interesting to pursue, but we're not quite there yet.

This revision now requires changes to proceed.Jan 26 2023, 9:00 AM
philnik abandoned this revision.Sep 14 2023, 9:23 AM