[PATCH] D81447: [llvm] Added support for stand-alone cmake object libraries.

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked an inline comment as done.
mtrofin added inline comments.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:390
+#     If STATIC or SHARED are
+#     also specified, create an OBJECT library target named obj.${name} in
+#     addition to those.
----------------
phosek wrote:
> I don't understand this part of the comment, does it mean that we always want to create `OBJECT` library even either `STATIC` or `SHARED` is specified?
This documents the existing behavior. Today, this is what happens:

llvm_add_library.   |. add_library parameter

STATIC                    |   STATIC
SHARED                 |   SHARED
STATIC SHARED.   |  SHARED, called ${name}, and a STATIC also, called ${name}_static
STATIC OBJECT.    |  STATIC called ${name}, and an OBJECT called obj.${name}. The link interface defined when calling llvm_add_library becomes 
                                    dependencies of this OBJECT target. The OBJECT target has no link interface 
SHARED OBJECT  | same as above, s/STATIC/SHARED
OBJECT                   | same as STATIC OBJECT. 

On this last point, my change is technically a breaking change - but it appears nothing uses it in that particular way, i.e. OBJECT without parameters. 

On this, I think we should enforce that at least one of the STATIC|OBJECT|SHARED be passed - wdyt?




================
Comment at: llvm/cmake/modules/AddLLVM.cmake:698
+# not referenced.
+macro (add_implementation_detail name)
+  cmake_parse_arguments(ARG "PUBLIC;PRIVATE" "" "" ${ARGN})
----------------
phosek wrote:
> I'm not a big fan of the name `add_implementation_detail` as it doesn't really match the rest of functions. How about `add_llvm_internal_library` or something along those lines?
sgtm!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81447/new/

https://reviews.llvm.org/D81447