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

[Tim Keith via Phabricator via llvm-commits]

tskeith requested changes to this revision.
tskeith added inline comments.
This revision now requires changes to proceed.


================
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)
----------------
sameeranjoshi wrote:
> 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
> 
> 
If the user requests unit tests and we can't run them it should be an error, not silently ignored.


================
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);
----------------
sameeranjoshi wrote:
> 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
This should be in a separate change. It has nothing to do with google test infrastructure.



================
Comment at: flang/runtime/io-error.cpp:10
 #include "io-error.h"
-#include "config.h"
 #include "magic-numbers.h"
----------------
sameeranjoshi wrote:
> tskeith wrote:
> > Is this supposed to be part of your change?
> Yes, out-of-tree builds fail with this include.
> Also see this commit addressing the same
> https://github.com/flang-compiler/f18-llvm-project/commit/4069068c997812c1c8ef6434a5e630603c7232e0
> 
This should be in a separate change. It has nothing to do with google test infrastructure.

The include of config.h was added for a reason. Does that reason no longer apply or are you breaking something else by changing this?


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

https://reviews.llvm.org/D80377