[PATCH] D101842: [flang][cmake] Enable the new driver by default

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/CMakeLists.txt:78
+
+option(FLANG_BUILD_NEW_DRIVER "Build the flang compiler driver" ON)
+if ("flang" IN_LIST LLVM_ENABLE_PROJECTS)
----------------
Could you move the option into the `if ("flang" IN_LIST LLVM_ENABLE_PROJECTS)` condition so at least it does not appear unless flang is enabled?


================
Comment at: llvm/CMakeLists.txt:84
+  endif()
+  if ( FLANG_BUILD_NEW_DRIVER AND NOT "clang" IN_LIST LLVM_ENABLE_PROJECTS)
+    message(STATUS "Enabling Clang as a dependency to flang")
----------------
[nit] Space between `(` and `FLANG_BUILD_NEW_DRIVER`. While not done consistently done in the file, the patch itself could itself be consistent with itself.


================
Comment at: llvm/CMakeLists.txt:86
+    message(STATUS "Enabling Clang as a dependency to flang")
+    list(APPEND LLVM_ENABLE_PROJECTS "clang")
+  endif()
----------------
`CACHE` variables are user-controlled and should not be modified. Actually, this creates a new non-cached local variables that shadows the cached variable.

>From the CMake manual:
> Normally projects should avoid using normal and cache variables of thesame name, as this interaction can be hard to follow.  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101842