[PATCH] D93778: [test-suite] Support Fortran tests in CMake infrastructure.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: CMakeLists.txt:7-11
+if (NOT TEST_SUITE_FORTRAN)
+  project(test-suite C CXX)
+else()
+  project(test-suite C CXX Fortran)
+endif()
----------------
Did you consider using `enable_language`? It supports an `OPTIONAL` flag for autodetect the presence of a Fortran compiler. (Not suggesting to use it, it might break buildbots that happen to have gfortran installed, and you will probably not want to run tests for a well-tested distribution provided compiler).



================
Comment at: CMakeLists.txt:30
   CMAKE_C_FLAGS CMAKE_CXX_FLAGS CMAKE_EXE_LINKER_FLAGS)
+if (TEST_SUITE_FORTRAN)
+    mark_as_advanced(CLEAR CMAKE_Fortran_COMPILER)
----------------
[Style] In other places in this file there is no space between `if` and `(`


================
Comment at: CMakeLists.txt:31
+if (TEST_SUITE_FORTRAN)
+    mark_as_advanced(CLEAR CMAKE_Fortran_COMPILER)
+endif()
----------------
There is probably a reason why the `_COMPILER` vars are marked as advanced, they cannot be changed after the initial configuration. I don't see a reason to make it non-advanced.


================
Comment at: CMakeLists.txt:42
+if (TEST_SUITE_FORTRAN)
+  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} ${TEST_SUITE_ARCH_FLAGS}")
+endif()
----------------
[not a change requirest] While good here for consistency with C/CXX, newer Cmake should use [[ https://cmake.org/cmake/help/latest/command/add_compile_definitions.html | add_compile_definitions ]] since `_FLAGS` is supposed to be controlled by the user, and `TEST_SUITE_EXTRA_Fortran_FLAGS` not be necessary.


================
Comment at: CMakeLists.txt:197
     "Extra flags for CMAKE_EXE_LINKER_FLAGS")
+
 mark_as_advanced(TEST_SUITE_EXTRA_C_FLAGS, TEST_SUITE_EXTRA_CXX_FLAGS,
----------------
[nit] unrelated whitespace change


================
Comment at: SingleSource/UnitTests/Fortran/CMakeLists.txt:1
+file(GLOB Source RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} *.f90 *.F90 *.f *.F)
+
----------------
Isn't this redundant with what `llvm_singlesource` does anyway?


Repository:
  rT test-suite

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

https://reviews.llvm.org/D93778