Ericson2314 added a comment.
@tstellar There wasn't a configuration necessitating these changes.
The CMake config file currently distinguishes a main include dir and secondary
include dir, which feels to me like it is leaking the implementation details of
which headers are generated or not. I w
tstellar added a comment.
> When LLVM is being built, the list is two elements long: generated headers
> and headers from source.
All these changes are guarded by if (CLANG_BUILT_STANDALONE), which means LLVM
should already be built. What build configuration are you using where you
needed thi
Ericson2314 added a comment.
I tested this in my distro build setup, and reread the LLVM source to make sure
the list was as I expected.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130553/new/
https://reviews.llvm.org/D130553
__
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcc56a5022c94: [clang][lld][cmake] Simplify header dirs
(authored by Ericson2314).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
Ericson2314 added a comment.
@sebastian-ne I didn't quote it because it is a list, but this might be shell
script habit sneaking through. I am not sure what is best for CMake.
Checking now,
https://stackoverflow.com/questions/35847655/when-should-i-quote-cmake-variables
says "If your variable
sebastian-ne accepted this revision.
sebastian-ne added a comment.
This revision is now accepted and ready to land.
Looks good to me, I left three questions inline.
Comment at: clang/CMakeLists.txt:81
# path is removed.
-set(MAIN_INCLUDE_DIR "${LLVM_INCLUDE_DIR}")
+
Ericson2314 added inline comments.
Comment at: clang/CMakeLists.txt:81
# path is removed.
-set(MAIN_INCLUDE_DIR "${LLVM_INCLUDE_DIR}")
+set(INCLUDE_DIRS ${LLVM_INCLUDE_DIRS})
set(LLVM_OBJ_DIR "${LLVM_BINARY_DIR}")
sebastian-ne wrote:
> Do we nee
sebastian-ne added inline comments.
Comment at: clang/CMakeLists.txt:81
# path is removed.
-set(MAIN_INCLUDE_DIR "${LLVM_INCLUDE_DIR}")
+set(INCLUDE_DIRS ${LLVM_INCLUDE_DIRS})
set(LLVM_OBJ_DIR "${LLVM_BINARY_DIR}")
Do we need to add `"${LLVM_BIN
Ericson2314 created this revision.
Ericson2314 added reviewers: beanz, sebastian-ne, mstorsjo.
Herald added a subscriber: mgorny.
Herald added a project: All.
Ericson2314 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
We don't need to reco
