sammccall added a comment.
In D121763#3385492 <https://reviews.llvm.org/D121763#3385492>, @hokein wrote:
> I think this patch is probably doing too much: 1. move CLANG_TOOLS_DIR to
> `configure_site_lit_cfg()` 2. rename.
>
> Can we split it into two patches? Land the first part, and the second part
> afterwards.
I don't think it can reasonably be called CLANG_TOOLS_DIR in that macro, the
var has no connection to clang, and is set even when clang is not enabled.
================
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}")
----------------
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? :-)
================
Comment at: llvm/utils/gn/secondary/clang-tools-extra/clangd/test/BUILD.gn:31
"CLANG_LIBS_DIR=", # needed only for shared builds
+ "CURRENT_TOOLS_DIR=",
----------------
hokein wrote:
> nit: this seems not needed anymore, as we have removed the `CLANG_LIBS_DIR`
> from the CMake file already.
Agree, I'll make a separate commit to avoid muddying the waters here.
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
