[PATCH] D24005: [compiler-rt cmake] Support overriding llvm-config query results

[Chris Bieneman via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

One comment below, otherwise LGTM.


================
Comment at: cmake/Modules/CompilerRTUtils.cmake:200
@@ -199,5 +199,3 @@
   string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" CONFIG_OUTPUT ${CONFIG_OUTPUT})
-  list(GET CONFIG_OUTPUT 0 LLVM_BINARY_DIR)
-  list(GET CONFIG_OUTPUT 1 LLVM_TOOLS_BINARY_DIR)
-  list(GET CONFIG_OUTPUT 2 LLVM_LIBRARY_DIR)
-  list(GET CONFIG_OUTPUT 3 LLVM_MAIN_SRC_DIR)
+  list(GET CONFIG_OUTPUT 0 LLVM_OBJ_ROOT)
+  list(GET CONFIG_OUTPUT 1 TOOLS_BINARY_DIR)
----------------
mgorny wrote:
> Hahnfeld wrote:
> > Is `BINARY_DIR` reserved in CMake? Otherwise I would prefer to name this consistently...
> I don't think it is. I used that name for consistency with llvm-config options (naming --obj-root BINARY_DIR actually confused me a lot at a first glance), and for consistency with the code used in clang (which names variables exactly like this).
> 
> Not saying I like changing names like this but I think if any change is due, we should do it everywhere consistently.
If you want to be consistent with the options (which makes sense), you should name it `OBJ_ROOT`, and also rename `TOOLS_BINARY_DIR` to `BINDIR`, `LIBRARY_DIR` to `LIBDIR`, and `MAIN_SRC_DIR` to `SRC_ROOT`. Being partially consistent doesn't make sense.


https://reviews.llvm.org/D24005