[PATCH] D80377: [flang] Google test infrastructure support for unittests

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi marked 10 inline comments as done.
sameeranjoshi added inline comments.


================
Comment at: flang/CMakeLists.txt:128
+  set(UNITTEST_DIR ${LLVM_BUILD_MAIN_SRC_DIR}/utils/unittest)
+  if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
+    if (TARGET gtest)
----------------
tskeith wrote:
> Why do you have to check for `gtest.h`? Won't it always be there?
Not always because

> 
> The out-of-tree build can be configured against the CMAKE_INSTALL_PREFIX directory of llvm, rather than the build directory. 
> LLVM does not install gtest when installing itself, meaning that gtest might not be available.
> 

Also other subprojects as well follow something similar
[1]https://github.com/llvm/llvm-project/blob/8e058feae0b0d07cd86257f0aa3154acfa887fe0/clang/CMakeLists.txt#L188
[2]https://github.com/llvm/llvm-project/blob/82aac878beb48cd326b4684918b7ff2375fae439/polly/CMakeLists.txt#L29
[3]https://github.com/llvm/llvm-project/blob/82aac878beb48cd326b4684918b7ff2375fae439/lld/CMakeLists.txt#L109




================
Comment at: flang/include/flang/Optimizer/Support/InternalNames.h:68
   std::string doConstant(llvm::ArrayRef<llvm::StringRef> modules,
+		         llvm::Optional<llvm::StringRef> host,
                          llvm::StringRef name);
----------------
tskeith wrote:
> This doesn't look properly formatted. Is it  part of your change?
Yes it is.
I will format it.
This was a bug fixed in fir-dev[1] just porting it here.
https://github.com/flang-compiler/f18-llvm-project/pull/87/files


================
Comment at: flang/test/Unit/lit.cfg.py:33
+# Propagate PYTHON_EXECUTABLE into the environment
+config.environment['PYTHON_EXECUTABLE'] = sys.executable
+
----------------
schweitz wrote:
> richard.barton.arm wrote:
> > Other sub-projects set this up in top-level CMakeLists using FindPythonInterp. Why do we set it here?
> Sameeran was pulling this glue code from the lld subproject, where these config settings are made. They can probably be deleted here. I do not appear to be used, but I haven't tested it.
Removed them as they are not used currently.


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

https://reviews.llvm.org/D80377