mstorsjo added inline comments.
================
Comment at: llvm/cmake/modules/AddLLVM.cmake:2401
+
+macro(setup_host_tool tool_name setting_name exe_var_name target_var_name)
+ cmake_parse_arguments(ARG "" "SCOPE" "" ${ARGN})
----------------
beanz wrote:
> Please make this a `function` instead of a `macro`. In general CMake `macros`
> expand in ways that are unintuitive which can be a maintenance burden for
> people coming in and modifying the code.
>
> Also a bit nity, but maybe a name like `find_host_program` would be more
> consistent naming. I suspect you could even simplify this implementation by
> using CMake's `find_program`
> (https://cmake.org/cmake/help/latest/command/find_program.html)
Re `function` vs `macro` - in `mlir-linalg-ods-gen`, we need to set the
variable in the parent scope (i.e. the CMakeLists.txt of the surrounding
directory); afaik with a `function` we can set the variable in the scope of the
caller, but not in the caller's parent - or am I missing something?
Re naming, I think `find_host_program` as name feels a bit unintuitive for the
regular (non-cross compiling) case; we've set up a target for building a tool
and we want to execute it at build time - plus doing the boilerplate setup of
building a separate host version of it when cross compiling. The fact that we
may want to use a preexisting binary, if the user has pointed us to it, is the
exception in the uncommon case here, so I wouldn't want to name the high level
structure based on that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131052/new/
https://reviews.llvm.org/D131052
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
