[libcxx-commits] [PATCH] D142285: [libc++] Don't include <version> everywhere

Sun Jan 22 06:40:06 PST 2023

Mordante added a comment.

In D142285#4071762 <https://reviews.llvm.org/D142285#4071762>, @philnik wrote:

> In D142285#4071753 <https://reviews.llvm.org/D142285#4071753>, @Mordante wrote:
>
>> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142285