[PATCH] D78306: [flang] Use LLVM's flags

[Steve Scalpone via Phabricator via llvm-commits]

sscalpone added inline comments.


================
Comment at: flang/CMakeLists.txt:70
   endif()
+  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
+  option(LLVM_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
----------------
How about only default to ON if the user has not already set LLVM_ENABLE_WARNING?  That lets the user say LLVM_ENABLE_WARNING=OFF.


================
Comment at: flang/CMakeLists.txt:71
+  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
+  option(LLVM_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
 
----------------
How about only default to ON if the user has not already set LLVM_ENABLE_PEDANTIC?


================
Comment at: flang/CMakeLists.txt:79
 
-  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
   option(LLVM_INSTALL_TOOLCHAIN_ONLY
----------------
How about preserving this line if LLVM_ENABLE_WARNINGS isn't otherwise set?  This way, people who don't want warnings can just -DLLVM_ENABLE_WARNING=OFF.


================
Comment at: flang/CMakeLists.txt:239
-# Add global F18 flags.
-set(CMAKE_CXX_FLAGS "-fno-rtti -fno-exceptions -pedantic -Wall -Wextra -Werror -Wcast-qual -Wimplicit-fallthrough -Wdelete-non-virtual-dtor ${CMAKE_CXX_FLAGS}")
 
----------------
It would be nice if these options were preserved with LLVM_COMPILER_IS_GCC_COMPATIBLE, beside -Werror, which is controlled elsewhere, and also, maybe -Wpedantic.


================
Comment at: flang/CMakeLists.txt:251
+  if( MSVC )
+    append("/WX" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+  endif()
----------------
Is there a tradition of documenting a single-letter option like /X?


================
Comment at: flang/lib/Optimizer/CMakeLists.txt:1
-# Sources generated by tablegen have unused parameters.
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-parameter")
----------------
Why not include this under LLVM_COMPILER_IS_GCC_COMPATIBLE?


================
Comment at: flang/unittests/Evaluate/CMakeLists.txt:92
+set(LLVM_REQUIRES_EH ON)
+set(LLVM_REQUIRES_RTTI ON)
 add_executable(real-test
----------------
Is rtti really needed here?


================
Comment at: flang/unittests/Runtime/CMakeLists.txt:5
+set(LLVM_REQUIRES_EH ON)
+set(LLVM_REQUIRES_RTTI ON)
 add_library(RuntimeTesting
----------------
Is rtti really needed here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78306