Including <ciso646> should result in an #error since C++17

[georgthegreat]

Summary

NB: this description is about iteration #1. Current iteration suggests another approach. I'll try not to forget to fix the description before commit (if it ever going to take place).

MSVC does not provide builtin support for alternative operator representations.

Instead, their #include <сiso646> includes iso646.h, which contains preprocessor definitions for these terms.

Generally, people, even LLVM project itself do not expect their code to be compiled agains libcxx on Windows. This PR makes #include <ciso646> and #include <iso646.h> equavalent.

[mclow.lists]

That cppreference page you quote says:

The same words are defined in the C programming language in the include file <iso646.h> as macros. Because in C++ these are built into the language, the C++ version of <iso646.h>, as well as <ciso646>, does not define anything.

[ldionne]

In D96786#2565638, @mclow.lists wrote:

That cppreference page you quote says:

The same words are defined in the C programming language in the include file <iso646.h> as macros. Because in C++ these are built into the language, the C++ version of <iso646.h>, as well as <ciso646>, does not define anything.

I agree with Marshall. I think the better approach here would be to file a bug against MSVC and ask them why they do not define the alternate operator representations as keywords in their compiler. If there's a good reason, we could go with your patch because it doesn't cost us much.

[CaseyCarter]

In D96786#2565654, @ldionne wrote:

In D96786#2565638, @mclow.lists wrote:

That cppreference page you quote says:

The same words are defined in the C programming language in the include file <iso646.h> as macros. Because in C++ these are built into the language, the C++ version of <iso646.h>, as well as <ciso646>, does not define anything.

I agree with Marshall. I think the better approach here would be to file a bug against MSVC and ask them why they do not define the alternate operator representations as keywords in their compiler. If there's a good reason, we could go with your patch because it doesn't cost us much.

MSVC only recognizes the alternative tokens in strict (/permissive-) mode to avoid breaking legacy code that uses those lexemes as identifiers. I have no opinion on whether or not libc++ should accept this change, but I strongly recommend that C++ code that wants portable conformant behavior when compiling with MSVC should use /permissive-, which will finally be on-by-default in C++20 mode.

[georgthegreat]

I have digged through C++ Working Draft and found the following:

16.4.5.3.2 Zombie names [zombie.names]

...

4 The header names <ccomplex>, <ciso646>, <cstdalign>, <cstdbool>, and <ctgmath> are reserved for
previous standardization.

C.1.10 Clause 16: library introduction [diff.cpp17.library]

...

2 Affected subclause: 16.4.2.3
Change: Remove vacuous C++ header files.
Rationale: The empty headers implied a false requirement to achieve C compatibility with the C++ headers.
Effect on original feature: A valid C++ 2017 program that #includes any of the following headers may
fail to compile: <ccomplex>, <ciso646>, <cstdalign>, <cstdbool>, and <ctgmath>. To retain the same
behavior:

...

(2.3) — a #include of <ciso646>, <cstdalign>, or <cstdbool> can simply be removed.

This makes me think that it maybe better to generate #error instead of trying to workaround MSVC problem.

@mclow.lists, @ldionne, what do you think?

[ldionne]

In D96786#2566105, @georgthegreat wrote:

I have digged through C++ Working Draft and found the following:

[...]

This makes me think that it maybe better to generate #error instead of trying to workaround MSVC problem.

@mclow.lists, @ldionne, what do you think?

I don't think we can #error out (or remove those headers, which would be equivalent) because we still need to support previous Standards. In C++03, for example, <ciso646> is a valid header, and although code should not use it, someone would be in their right to file a bug against our C++03 conformance if we didn't provide it.

I 100% agree in spirit that we should remove these headers, though. Just not in practice.

[georgthegreat]

@ldionne, actually, I was talking about standard-bound #error, as follows:

#if _LIBCPP_STD_VER > 14
#error "#include <ciso646> is not valid in C++17"
#endif

I am not sure, what would be the most portable way to generate #error though.

[georgthegreat]

MSVC only recognizes the alternative tokens in strict (/permissive-) mode to avoid breaking legacy code that uses those lexemes as identifiers. I have no opinion on whether or not libc++ should accept this change, but I strongly recommend that C++ code that wants portable conformant behavior when compiling with MSVC should use /permissive-, which will finally be on-by-default in C++20 mode.

It looks like /permissive- did the trick. Thanks for pointing this option out. I will try to integrate this option into build process.

Current change is definitely incorrect, including this header should break compilation if the code is compiled against modern C++ standard.

[georgthegreat]

@ldionne, I have changed the behavior to cause an error if the user includes <ciso646> expecting certain behavior from the standard library.

I can also confirm that MSVC /permissive- compilation flag is working well (though it causes various other errors in large codebases).

If this diff looks fine, I can fix the rest of the headers marked as reserved by the standard.

[ldionne]

I'm unsure of the value of doing this vs the amount of breakage it may cause.

There's no doubt that your change is in line with the Standard, I'm just wondering whether it would cause wide trouble without having that much of a practical benefit. In practice, I'm pretty sure this is going to break some open source libraries like Boost which I think have been using #include <ciso646> to detect the standard library (that used to be true for some Boost libraries at some point, at least). Also, both MSVC and libstdc++ do provide those headers.

@mclow.lists do you have an opinion about this change?

[georgthegreat]

NB: At the time <ciso646> already causes trouble. People are expecting it to behave just like <iso646.h>. This works for most of the headers — you can replace <string.h> with <cstring> and get string.h transitively included, but does not work with this one.

Iteration #1 was about restoring the status quo, iteration #2 is about making libcxx follow the C++ standard.

[ldionne]

Please make the change in all the headers (<cstdbool>, etc.). I'll then take that change and compile a significant code base internally with it and we'll see how much trouble it causes. That'll allow us to better evaluate the disruptive potential for this change.

libcxx/include/ciso646

21

Also, please use the following wording:

#error "C++17 removed the <ciso646> header. If you're trying to detect the Standard library, include <version> instead"

You could also add similar wording for the other headers (for example tell users to use <stdbool.h> instead of <cstdbool>).

[georgthegreat]

I have implemented similar patch in all headers mentioned in C++ working draft as reserved / removed.

<cstdalign> in not present in libc++ though marked as removed only in C++20.

[mclow.lists]

(2.3) — a #include of <ciso646>, <cstdalign>, or <cstdbool> can simply be removed.

This makes me think that it maybe better to generate #error instead of trying to workaround MSVC problem.

@mclow.lists, @ldionne, what do you think?

I disagree. We have given people guidance that #include <ciso646> is a useful way to identify which standard library you are using.
See https://stackoverflow.com/questions/31657499/how-to-detect-stdlib-libc-in-the-preprocessor as an example.

[georgthegreat]

Ok, I have run CI check on a large codebase too.

<ciso646> is a real problem here, but I see no real world inclusions of either <ctgmath>, <cstdbool> or <ccomplex>.

How about fixing these three, leaving <ciso646> unchanged? As far, as I see, there is no legal replacement for <ciso646> in C++17, as <version> only appears in C++20.

[ldionne]

Honestly I'm pretty wary of doing this. The benefit is very small (being pedantic and forcing users to migrate away from those headers), but the potential for breakage exists.

In general, we don't really like erroring out in headers like that because it defeats the expectation that users can test for whether a header can be included with __has_include. Yes, I know, they're wrong to think that __has_include(<HEADER>) means that <HEADER> can be meaningfully included, but people do that.

As much as I like being pedantic, I think this is not worth doing right now. Maybe if we can get some buy-in from other implementations, then it would make more sense. Otherwise, we're just going to be a nuisance for the ecosystem.

[georgthegreat]

Ok, I am discarding this PR for the time being.

libcxx/include/ciso646

 // -*- C++ -*-
 //===--------------------------- ciso646 ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP_CISO646
 #define _LIBCPP_CISO646
 
 /*
     ciso646 synopsis
+
+*/
+
+#include <__config>
 
-*/
-
-#include <__config>
+#if _LIBCPP_STD_VER > 14
+#error "Including <ciso646> is forbidden since C++17"
+#endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 #endif  // _LIBCPP_CISO646