[PATCH] D138258: clang/cmake: Fix incorrectly disabling tests when LLVM_EXTERNAL_LIT is used
Tom Stellard via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 18 09:31:21 PST 2022
tstellar added inline comments.
================
Comment at: clang/CMakeLists.txt:97
# Seek installed Lit.
- find_program(LLVM_LIT
- NAMES llvm-lit lit.py lit
- PATHS "${LLVM_MAIN_SRC_DIR}/utils/lit"
- DOC "Path to lit.py")
+ if (NOT LLVM_EXTERNAL_LIT)
+ find_program(LLVM_EXTERNAL_LIT
----------------
kwk wrote:
> mgorny wrote:
> > I don't think you need to do this if you rename the var, `find_program()` doesn't seem to do any searching when the var is already set.
> >
> > I've tested with a dummy CMakeLists:
> >
> > ```
> > set(FOO /bin/true)
> > find_program(FOO NAMES foo)
> > message(FATAL_ERROR ${FOO})
> > ```
> >
> > gives `/bin/true` for me.
> > I don't think you need to do this if you rename the var, `find_program()` doesn't seem to do any searching when the var is already set.
>
> This might be true but it is counter intuitive to assume that `find_program` does nothing. So for readability I'd keep the `if (NOT LLVM_EXTERNAL_LIT)`. I know it is not needed but with the `if` it won't look like `find_program(LLVM_EXTERNAL_LIT` is the only/central place to set the variable `LLVM_EXTERNAL_LIT`. With the guard around it, it becomes more obvious that this is more of a fallback.
@mgorny The [[ http://devdoc.net/linux/cmake-3.9.6/command/find_program.html | documentation ]] confirms this too:
"Once one of the calls succeeds the result variable will be set and stored in the cache so that no call will search again."
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138258/new/
https://reviews.llvm.org/D138258
More information about the cfe-commits
mailing list