Details
- Reviewers
jdoerfert DavidTruby sscalpone aaron.ballman - Group Reviewers
Restricted Project
Diff Detail
Event Timeline
Agreed, these should be (and are!) checked in the CMake so this doesn't really serve a purpose here. LGTM!
As an aside, can you in future upload the diff with -U99999 so we can see the whole context in Phabricator? Thanks!
What is the motivation for this change? Is there a positive change that could be made instead, that would achieve the same goal and preserve the functionality of this code?
To make it compile with MSVC.
Is there a positive change that could be made instead, that would achieve the same goal and preserve the functionality of this code?
The goal is already achieved in cmake files.
Are you able to wrap the original code in such a way that it doesn't fail under VC++?
Do you mean whole Flang or this particular case?
Just this case. I mean, preserve the original code in this change for non-windows systems.
We can add a compilation flag /Zc:__cplusplus to make MSVC correctly report __cplusplus .
I still don't really understand why we need these checks when they're already performed by CMake; do you have an example where the checks in CMake are insufficient?
I don't think this preserves the intended behavior of the original code -- now MSVC will never check for C++17 mode. I believe the correct way to fix this is to set /Zc:__cplusplus when compiling with MSVC, unless there's a reason that's not possible?
Well, C++17 mode is already checked with CMake, so there is no need to check it with the compiler. I excluded MSVC just because @sscalpone insisted to preserve the original code.
There's a small chance someone could skip CMake entirely and use these files directly (that happens with LLVM support code from time to time, for instance), so I think preserving the behavior of the original code is a reasonable idea. If we set up the CMake to tell MSVC to use the right value for __cplusplus, then the code in idioms.h requires no changes and will do the right thing.
/Zc:__cplusplus is only available with Visual Studio >= 15.7. the code requires a change in a way or another.
That's MSVC 2017, which is already our base requirement (we require Visual Studio 2017 with the latest updates installed according to https://llvm.org/docs/GettingStartedVS.html).
LLVM headers need fixes to make sure they work with MSVC setting __cplusplus to anything above 201402. For eg: https://reviews.llvm.org/D76997