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

[Isuru Fernando via Phabricator via llvm-commits]

isuruf marked 7 inline comments as done.
isuruf 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)
----------------
sscalpone wrote:
> 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.
That is how CMake options work. The value we are setting here is a default and the user can override it on the command line.


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


================
Comment at: flang/CMakeLists.txt:79
 
-  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
   option(LLVM_INSTALL_TOOLCHAIN_ONLY
----------------
sscalpone wrote:
> 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.
On a in-tree build, this option is already defined. In an out-of-tree build, this option is defined in Line 70


================
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}")
 
----------------
sscalpone wrote:
> 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.
`-Wall -Wextra -Wcast-qual -Wimplicit-fallthrough -Wdelete-non-virtual-dtor` are all covered by `LLVM_ENABLE_WARNINGS=ON` which is the default.
`-fno-rtti` is covered by `LLVM_ENABLE_RTTI=OFF` which is the default.
`-fno-exceptions` is covered by `LLVM_ENABLE_EH=OFF` which is the default.



================
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")
----------------
sscalpone wrote:
> Why not include this under LLVM_COMPILER_IS_GCC_COMPATIBLE?
`-Wno-unused-parameter` is added by `LLVM_ENABLE_WARNINGS`


================
Comment at: flang/unittests/Evaluate/CMakeLists.txt:92
+set(LLVM_REQUIRES_EH ON)
+set(LLVM_REQUIRES_RTTI ON)
 add_executable(real-test
----------------
sscalpone wrote:
> Is rtti really needed here?
Otherwise, there's a warning. See https://github.com/llvm/llvm-project/blob/6701993027f8af172d7ba697884459261b00e3c6/llvm/cmake/modules/AddLLVM.cmake#L17


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78306