sammccall 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}")
----------------
hokein wrote:
> 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?
>
> > 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).
That's right. Ultimately it's being used to make sure that when you run lit
tests, `clang` resolves to the clang you just built and not something else.
(It's more complicated than that, there are several conflicting mechanisms, but
that's the essence).
> > 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?
It really means the current cmake invocation.
In a "standalone build" of clang (rooted at llvm-project/clang, with a separate
LLVM tree), it is `build/bin` where `LLVM_TOOLS_DIR` is `path/to/llvm/bin`, and
was produced by a *previous* cmake invocation (or an install step).
In a normal unified build of llvm+clang+lld (rooted at llvm-project/llvm with
LLVM_ENABLE_PROJECTS), both variables are set to `build/bin`.
There's really no fixed connection with a subproject at all, and certainly its
value doesn't vary across the various subprojects of llvm-project.
> I think it'd be better to have the PROJECT in the variable name,
> `LLVM_PROJECT_TOOLS_DIR`
I think this name is misleading because of the connection to the repo
`llvm-project`.
It suggests `LLVM_PROJECT_TOOLS_DIR` is a top-level thing when it's really the
opposite, a subproject thing. (And while `LLVM_TOOLS_DIR` *does* correspond to
`llvm-project/llvm` the tools are often things people think of as "top-level").
> or shorter `PROJECT_TOOLS_DIR`. WDYT?
I can live with it, but I'm not really sure whether `PROJECT` communicates
anything that's true :-)
I'm tempted by just `TOOLS_DIR` but I worry that such a short name with a lack
of namespacing is asking for trouble. On the other hand, there's `SHLIBDIR`,
which also points at shared libs in the current build tree...
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
