[libcxx-commits] [PATCH] D92212: Make libcxx work according to Clang C++ Status if -fchar8_t is passed

Tue Dec 1 11:50:06 PST 2020

ldionne commandeered this revision.
ldionne edited reviewers, added: georgthegreat; removed: ldionne.
ldionne added a comment.
Herald added a subscriber: jkorous.

In D92212#2421060 <https://reviews.llvm.org/D92212#2421060>, @georgthegreat wrote:

> @ldionne, this PR appeared as a part of a large monorepo migration to a new standard.
> If I just change -std=c++17 to -std=c++20, thousands of new errors appear (even upon switching off all the major deprecation warnings), so I am trying to split the switch into smaller parts and fix them one by one.

The usual way I've seen migrations being done is by fixing the code such that it works both in C++17 and C++20. You iterate locally, fixing issues and committing them until the whole code base compiles both as C++17 and C++20. You then switch the compiler to `-std=c++20`, and can get rid of any C++17 specific workarounds you introduced.

Requiring the compiler and standard library to provide some sort of mode `-std=c++17-but-with-char8_t` is the wrong way to do this. What if you did not even have the `-fchar8_t` flag? Would you implement it just to ease your transition? The bottom line is that we don't want to backport arbitrary C++20 features to C++17 to ease code base migrations -- that's just not the right way to make these migrations.

If you're wondering why I'm having such a strong stance, take a look at libc++'s attempt to provide C++11 features in C++03. While it had value initially and I understand why that choice was made at the beginning, it has become a terrible mess that we're stuck with. There's bugs and ABI incompatibilities in it, and it adds a lot of complexity for relatively little benefit since C++11 has been around for almost 10 years. Of course, that is made worse by the fact that C++03 and C++11 were greatly different (so 11 features were difficult to implement in 03), however the principle is the same. The library and language are evolved as a coherent whole, and trying to pull features in or out of a given standard mode is always a challenge, and it adds a maintenance burden.

> I think that WG21 invented feature testing macros for a reason.

Indeed, but also note that feature test macros were a highly contentious issue, and not all implementers were very favourable to their introduction. But they were standardized, and so libc++ does implement them. However, implementing feature test macros does not imply that the library should start giving meaning to arbitrary combinations of flags (in this case `-std=c++17 -fchar8_t`). The trap we really want to avoid is what I illustrated above with C++11 features in C++03 mode.

I think that using the compiler's feature test macros in libc++ to conditionally enable library features is reasonable in some cases (e.g. when the library feature is fairly self contained, etc), and for limited times (e.g. until the oldest compiler we support implements the feature). Once our support window moves and the oldest compiler we support definitely supports the feature in the right standard mode, I think it's a lot better to get rid of these conditionals and instead assume the compiler feature works when the right standard is set. This reduces the complexity of the code by removing `#ifdefs`, which is the source of a lot of our woes in a codebase like libc++. It also reduces the strain on the test suite, which has to be annotated for everything that we conditionally support.

Other unrelated comments for the next time:

1. Please make sure the LLVM monorepo is added to your Phabricator revision, otherwise CI doesn't kick in.
2. Please add context to the diff you upload to Phabricator.

I'm going to commandeer this revision to suggest an approach that should solve your issue, while being OK with me (I can't speak for Marshall, though). Basically, I will:

1. Conditionalize `__cpp_lib_char8_t` on whether `__cpp_char8_t` is defined, meaning the compiler supports the feature. I won't look at the Standard version.
2. Use `__cpp_lib_char8_t` consistently instead of `_LIBCPP_NO_HAS_CHAR8_T` -- my personal opinion is that using standardized macros when we can is easier to grok than libc++ defining its own. This seems to be an opinion shared by not all, but some people.
3. Once we've straightened our compiler requirements (which are currently way outdated), I will switch to defining `__cpp_lib_char8_t` solely based on the version of `__cplusplus`, and will update/simplify the conditionals that use `__cpp_lib_char8_t` inside the library whenever possible.

I'd like to know what you folks think about this plan.

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

https://reviews.llvm.org/D92212