Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove #include <ciso646> #2089

Closed
igfraga opened this issue May 6, 2020 · 12 comments · Fixed by #2115
Closed

Remove #include <ciso646> #2089

igfraga opened this issue May 6, 2020 · 12 comments · Fixed by #2115
Assignees
Labels
kind: enhancement/improvement release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@igfraga
Copy link

igfraga commented May 6, 2020

Was a bit of a surprise that 'or' and 'and' work in our code base and tracked it down to files that include nlohmann/json. Also note this header is going away in C++20.

@jaredgrubb
Copy link
Contributor

What do you mean? and and or have always been keywords in C++.

@ArtemSarmini
Copy link
Contributor

They are not in msvc without /permissive-. It it set by default in VS 15.5 and above, but old projects may break if we remove this header.

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

Indeed we include the header due to old MSVC versions. We should check if we can detect these versions with a macro (Hedley has a lot of them...) and only include it then.

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

Related: #1782

@ArtemSarmini
Copy link
Contributor

Problem is this option does not depend on msvc version, that is, old project with permissive setting can be built with newest msvc and vice versa. AFAIK there's no macro to check it. But we can check build flags and set some macro ourselves. @nlohmann sounds good?

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

@ArtemSarmini You are fast! I just wanted to let you know about the change and ask for your opinion. An alternative to this approach would be to detect C++20 and not try to include if C++20 is used. Would that be feasible?

@ArtemSarmini
Copy link
Contributor

Again this flag does not depend on C++ version.
I suppose many projects nowdays switch this flag off. But I don't know how many of library users have it on (and will be affected when we remove the include) and don't know how to find it out either. I'll make a poll in a local community but it may be unrepresentative.

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

But in C++20, the header disappears, so I would expect that alternative operator representations must then work without that header. Or to put it differently: is there a minimal version that does not need <ciso646> any more?

@ArtemSarmini
Copy link
Contributor

ArtemSarmini commented May 7, 2020

alternative operator representations must then work without that header

No idea. I'll try to ask STL about that. But I guess microsoft assumes projects either use old msvc version or have the flag off.

is there a minimal version that does not need any more?

As I understand it there's no such version.

And it doesn't disappear, it is deprecated and (according to comitee strategy) will be removed in 23. That said, vendors may leave it for compatibilty. We still should not use it though.

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

It's deprecated since C++17 and will be removed in C++20 (see https://en.cppreference.com/w/cpp/header/ciso646, http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2131r0.html).

So basically

#if __cplusplus <= 201703L
    #include <ciso646> // and, not, or
#endif

should do the job.

@ArtemSarmini
Copy link
Contributor

I was wrong about version.
I was suggested a cool trick which should do it, I'll test it locally first and let you know.

@nlohmann
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants