[PATCH] D100442: [flang][msvc] Fix compilation of RuntimeGtest tests.

Thu Apr 15 13:33:57 PDT 2021

ashermancinelli accepted this revision.
ashermancinelli added inline comments.
This revision is now accepted and ready to land.

================
Comment at: flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp:31
   static bool isCrashHanlderRegistered{false};
-  if (not isCrashHanlderRegistered)
+  if (!isCrashHanlderRegistered)
     Fortran::runtime::Terminator::RegisterCrashHandler(CatchCrash);
----------------
Meinersbur wrote:
> ashermancinelli wrote:
> > MehdiChinoune wrote:
> > > tskeith wrote:
> > > > awarzynski wrote:
> > > > > MehdiChinoune wrote:
> > > > > > awarzynski wrote:
> > > > > > > Is this change required to fix the build?
> > > > > > > Is this change required to fix the build?
> > > > > > 
> > > > > > Yes
> > > > > > 
> > > > > > ```
> > > > > > [2/34] Building CXX object tools\flang\unittests\RuntimeGTest\CMakeFiles\FlangRuntimeTests.dir\CrashHandlerFixture.cpp.obj
> > > > > > FAILED: tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/CrashHandlerFixture.cpp.obj
> > > > > > C:\PROGRA~2\MICROS~2\2019\COMMUN~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\flang\unittests\RuntimeGTest -ID:\dev\llvm-project\flang\unittests\RuntimeGTest -ID:\dev\llvm-project\flang\include -Itools\flang\include -Iinclude -ID:\dev\llvm-project\llvm\include -ID:\dev\llvm-project\llvm\utils\unittest\googletest\include -ID:\dev\llvm-project\llvm\utils\unittest\googlemock\include -ID:\dev\llvm-project\llvm\..\mlir\include -Itools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Fotools\flang\unittests\RuntimeGTest\CMakeFiles\FlangRuntimeTests.dir\CrashHandlerFixture.cpp.obj /Fdtools\flang\unittests\RuntimeGTest\CMakeFiles\FlangRuntimeTests.dir\ /FS -c D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp
> > > > > > D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(31): error C2065: 'not': undeclared identifier
> > > > > > D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(31): error C2146: syntax error: missing ')' before identifier 'isCrashHanlderRegistered'
> > > > > > D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(31): error C2059: syntax error: ')'
> > > > > > D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(32): error C2143: syntax error: missing ';' before 'Fortran::runtime::Terminator::RegisterCrashHandler'
> > > > > > ```
> > > > > I'm not familiar with MSVC, but AFAIK, `not` is a fairly standard C++ keyword. It's not clear to my *why* this change is needed. I appreciate that it fixes things for you, but it feels a bit random.
> > > > > 
> > > > > Just to clarify, I have no objections to this change, but I would appreciate a bit more justification, ideally with a reference. Something that would clarify *why*. This could be a very short explanation in the commit message.
> > > > MSVC requires `/permissive-` or `/Za` to get alternative operators, according to this: https://docs.microsoft.com/en-us/cpp/cpp/cpp-built-in-operators-precedence-and-associativity
> > > > 
> > > > That seems strange as they have been part of C++ for a long time, but I don't think we have any reason to use them anyway.
> > > > but AFAIK, `not` is a fairly standard C++ keyword.
> > > 
> > > Reference please!
> > > 
> > [[ https://en.cppreference.com/w/cpp/keyword/not | Here's a link on cppref ]], without any warning of `not` being nonstandard. I support the change especially if it fixes builds with MSVC, but it does seem strange that MSVC would not enable this by default.
> MSVC supports them when adding 
> ```
> #include <iso646.h>
> ```
> https://www.cplusplus.com/reference/ciso646/
> 
> or when compiling with `/Za` ([[ http://clang-developers.42468.n3.nabble.com/MSVC-Za-considered-harmful-td4024306.html | not recommended ]]). 
> 
> They part of C++ even without this header since day one, ie. MSVC is not standards compliant here.
> 
> However, in the interest of a common code style, we should prefer `!` over `not` even if compatibility was not an issue. 
It seems like we can agree on `not` -> `!`. I wouldn't mind adding a line in `flang/docs/C++style.md` about alternative operator spellings.

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

https://reviews.llvm.org/D100442