hokein added inline comments.
================
Comment at: llvm/cmake/modules/AddLLVM.cmake:1640
+ # Like LLVM_{TOOLS,LIBS}_DIR, but pointing at the build tree.
+ string(REPLACE "${CMAKE_CFG_INTDIR}" "${LLVM_BUILD_MODE}" CURRENT_TOOLS_DIR
"${LLVM_RUNTIME_OUTPUT_INTDIR}")
+ string(REPLACE "${CMAKE_CFG_INTDIR}" "${LLVM_BUILD_MODE}" CURRENT_LIBS_DIR
"${LLVM_LIBRARY_OUTPUT_INTDIR}")
----------------
sammccall wrote:
> hokein wrote:
> > the change looks good, but I'm not sure the new name `CURRENT_TOOLS_DIR` is
> > good, the name seems vague and not significantly better, since it always
> > points at the build tree, maybe `LLVM_BUILD_DIR` or something similar? (The
> > existing naming pattern looks like project-specific variables are prefixed
> > with the project name `LLVM_`, `CLANG_`).
> I agree the new name isn't good. (OTOH *all* these variables seem to have
> terrible names, so maybe it's just hard). But I think `LLVM_BUILD_DIR` is
> worse.
>
> The name needs to communicate a lot of things:
> 1. that this is parallel to `LLVM_TOOLS_DIR`, and the successor to
> `$PROJECT_TOOLS_DIR`
> 2. that when LLVM and $PROJECT are built in different paths, this is the dir
> for $PROJECT and not for LLVM
> 3. That this is not a concrete directory, but may contain a `%(build_mode)%`
> placeholder
> 4. this variable is part of llvm-project, as opposed to some other CMake
> project it may be mixed with.
>
> `CLANG_TOOLS_DIR` passes 1, 2, 4 (and fails 3, which is really hard).
> `CURRENT_TOOLS_DIR` passes 1 & 2 only - it lacks namespacing.
> `LLVM_BUILD_DIR` passes 4 only, I think - it's very unclear how it relates to
> `LLVM_TOOLS_DIR`.
> `LLVM_CURRENT_TOOLS_DIR` would pass 1 and 4, but muddies the
> LLVM-vs-subproject distinction a bit.
>
> What do you think of these criteria, are there some I missed? Any more name
> ideas? :-)
> The name needs to communicate a lot of things:
> 1. that this is parallel to LLVM_TOOLS_DIR, and the successor to
> $PROJECT_TOOLS_DIR
> 2. that when LLVM and $PROJECT are built in different paths, this is the dir
> for $PROJECT and not for LLVM
> 3. That this is not a concrete directory, but may contain a %(build_mode)%
> placeholder
> 4. this variable is part of llvm-project, as opposed to some other CMake
> project it may be mixed with.
Thanks, these make sense, I were not aware of all of these and didn't get the
full picture. I suppose it is being used for exposing various
`config.{project}_tools_dir` variables for different tools (e.g.
`config.clang_tools_dir`, `config.lldb_tools_dir` etc).
> CURRENT_TOOLS_DIR passes 1 & 2 only - it lacks namespacing.
hmm, I think the new name is somewhat weak on 2 (particularly the PROJECT bit)
-- the `Current` was unclear to me, I guess it means the current project right?
I think it'd be better to have the PROJECT in the variable name,
`LLVM_PROJECT_TOOLS_DIR` or shorter `PROJECT_TOOLS_DIR`. WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121763/new/
https://reviews.llvm.org/D121763
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
