[PATCH] D80377: [flang] Google test infrastructure support for unittests
Tim Keith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 4 12:09:39 PDT 2020
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
More information about the llvm-commits
mailing list