Page MenuHomePhabricator

[flang] Don't check __cplusplus value with MSVC.
Needs RevisionPublic

Authored by ChinouneMehdi on Apr 15 2020, 2:36 AM.

Diff Detail

Event Timeline

ChinouneMehdi created this revision.Apr 15 2020, 2:36 AM
ChinouneMehdi created this object with visibility "All Users".
ChinouneMehdi created this object with edit policy "Subscribers".
Herald added a project: Restricted Project. · View Herald Transcript
DavidTruby accepted this revision.Apr 15 2020, 6:28 AM
DavidTruby added a subscriber: DavidTruby.

Agreed, these should be (and are!) checked in the CMake so this doesn't really serve a purpose here. LGTM!

This revision is now accepted and ready to land.Apr 15 2020, 6:28 AM

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?

ChinouneMehdi added a comment.EditedApr 15 2020, 12:18 PM

What is the motivation for this change?

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.

ChinouneMehdi changed the visibility from "All Users" to "Public (No Login Required)".Apr 15 2020, 2:07 PM
ChinouneMehdi changed the edit policy from "Subscribers" to "All Users".Apr 17 2020, 11:48 AM

Are you able to wrap the original code in such a way that it doesn't fail under VC++?

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?

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.

This comment was removed by ChinouneMehdi.

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 .

Please, Someone commit this change.
email: chinoune.mehdi@hotmail.com

Please preserve the original code in this change for non-windows systems.

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?

ChinouneMehdi updated this revision to Diff 261355.EditedApr 30 2020, 2:27 PM
ChinouneMehdi retitled this revision from [flang] Remove some unnecessary checks. to [flang] Don't check __cplusplus value with MSVC..

Preserve the original code.

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 .

This seems like the correct way to solve this bug, to me.

sscalpone accepted this revision.May 4 2020, 11:28 PM
aaron.ballman requested changes to this revision.May 5 2020, 4:31 AM

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?

This revision now requires changes to proceed.May 5 2020, 4:31 AM

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.

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.

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.

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).

isuruf added a subscriber: isuruf.May 5 2020, 6:28 AM

LLVM headers need fixes to make sure they work with MSVC setting __cplusplus to anything above 201402. For eg: https://reviews.llvm.org/D76997

@isuruf Could you add /Zc:__cplusplus to MSVC flags with D78306.