MaskRay added a comment.
Herald added subscribers: bviyer, ekilmer, jplehr, thopre.
@Ericson2314 @phosek What's the state of this patch?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132608/new/
https://reviews.llvm.org/D132608
__
Ericson2314 added a comment.
I have done the deduping @phosek requested, and changed the variable names from
`CMAKE_*` to `LLVMPROJ_*` which hopefully satisfies everyone's criteria. Happy
with other non `LLVM_` options too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
htt
Ericson2314 updated this revision to Diff 473679.
Ericson2314 added a comment.
Herald added a subscriber: Moerafaat.
Dedup code, rename variables
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132608/new/
https://reviews.llvm.org/D132608
Files:
b
phosek added a comment.
I agree with @tianshilei1992, I think we should avoid introducing new `CMAKE_`
variables to avoid confusion. The same applies to module names, for example I
don't think we should be introducing `GNUBinaryDirs` which can be easily
confused for `GNUInstallDirs`.
I would p
Ericson2314 added a comment.
@tianshilei1992 that is a fair point. I would be open to calling them something
else.
I just don't want to call them `LLVM_*` because that would be confusing since
there is (a) LLVM in particular (b) the monorepo / project as a whole, and this
variable are about *n
tianshilei1992 added a comment.
Is it a good idea to define variables starting with `CMAKE`? That might be
confusing for later maintenance by other developers because chances are that
they will think those variables are CMake provided variables, try to look up
into CMake document to see what th
Ericson2314 added inline comments.
Comment at: libcxx/CMakeLists.txt:421
+if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+ set(default_install_path
"${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1")
+else()
ldionne wrote:
> ldionne wrote:
ldionne added inline comments.
Comment at: libcxx/CMakeLists.txt:421
+if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+ set(default_install_path
"${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1")
+else()
ldionne wrote:
> Instead, I'd do th
Ericson2314 updated this revision to Diff 460191.
Ericson2314 added a comment.
Fix misspelled variable `CLANG_CURRENT_SOURCE_DIR` -> `CLANG_SOURCE_DIR`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132608/new/
https://reviews.llvm.org/D132608
File
Ericson2314 updated this revision to Diff 460099.
Ericson2314 added a comment.
Fix typo in compiler-rt
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132608/new/
https://reviews.llvm.org/D132608
Files:
bolt/CMakeLists.txt
clang/CMakeLists.txt
Ericson2314 added inline comments.
Comment at: cmake/Modules/GNUBinaryDirs.cmake:3
+ get_filename_component(CMAKE_LIBDIR_BASENAME "${CMAKE_INSTALL_LIBDIR}" NAME)
+endif()
+
compnerd wrote:
> Should this perhaps be moved further down near the usage?
Sure
==
Ericson2314 updated this revision to Diff 459985.
Ericson2314 marked 4 inline comments as done.
Ericson2314 added a comment.
Rebase, avoid sed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132608/new/
https://reviews.llvm.org/D132608
Files:
bolt
ldionne added a comment.
I think I like the spirit of this change, which seems to be to move us closer
to `CMAKE_foo` variables and further from `LLVM_foo` variables for equivalent
functionality. I have a comment, but this essentially LGTM. The libc++ CI
failures (in particular the bootstrappin
compnerd added inline comments.
Comment at: cmake/Modules/GNUBinaryDirs.cmake:3
+ get_filename_component(CMAKE_LIBDIR_BASENAME "${CMAKE_INSTALL_LIBDIR}" NAME)
+endif()
+
Should this perhaps be moved further down near the usage?
Comment at: cma
sebastian-ne added a comment.
I’m not sure if it’s the case for all places (as `CMAKE_CFG_INTDIR` is not
defined at install time), but I think the `CMAKE_BINARY_LIBDIR` introduced here
serves the same purpose as the already existing `LLVM_LIBRARY_DIR`.
Same for `CMAKE_BINARY_INCLUDEDIR`, which i
arichardson added inline comments.
Comment at: cmake/Modules/GNUBinaryDirs.cmake:6
+if (NOT DEFINED CMAKE_BINARY_BINDIR)
+ set(CMAKE_BINARY_BINDIR "${CMAKE_BINARY_BINDIR}/bin")
+endif()
I find this name a bit confusing, maybe something like `CMAKE_BUILD_BINDIR`
Ericson2314 created this revision.
Ericson2314 added reviewers: sebastian-ne, beanz, phosek, ldionne.
Herald added subscribers: libc-commits, libcxx-commits, Enna1, bzcheeseman,
pmatos, asb, ayermolo, sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov,
tatianashp, msifontes, jurahul, Kayjukh
17 matches
Mail list logo
