This is an archive of the discontinued LLVM Phabricator instance.

Use object library if cmake supports it
ClosedPublic

Authored by tatyana-krasnukha on Jun 19 2019, 5:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

labath added a subscriber: labath.Jun 19 2019, 5:28 AM

why not just change lldb-mi-utils into a conventional (STATIC) library, at which point it can just be linked in using regular target_link_libraries, which has worked since forever?

It will cause some time overhead for linking a library. Linking is already formidable for the project and I'd avoid it when possible.

hintonda added inline comments.Jun 19 2019, 7:27 AM
tools/lldb/unittests/tools/lldb-mi/utils/CMakeLists.txt
16 ↗(On Diff #205556)

I'm assuming from your comment, that this is a performance issue. If so, could you add a comment and also update the commit message so people can appreciate the need for the added complexity?

Btw, just tested with 3.4.3, and your change made cmake happy. Thanks!

hintonda accepted this revision.Jun 19 2019, 7:28 AM

Otherwise, LGTM, thanks!

This revision is now accepted and ready to land.Jun 19 2019, 7:28 AM

I just reviewed all the release notes, and while 3.9 relaxed the usage of TARGET_OBJECTS, cmake doesn't explicitly mention allowing them in target_link_libraries until release 3.15.

I don't have any of those versions, but it looks like 3.15 is probably the version you need to use instead of 3.9. Please see: https://cmake.org/cmake/help/latest/release/3.15.html

tatyana-krasnukha retitled this revision from Add a worlaround for unsupported cmake feature to Use object library if cmake supports it.

As I figured out, cmake allows to use $<TARGET_OBJECTS:...> anywhere since version 3.9 (commit).

After the version 3.12 (commit) object libraries may be used as a right-hand side of target_link_libraries.

Version 3.15 allows to use $<TARGET_OBJECTS:...> expression for other types of libraries, but this change doesn't affect the patch.

Finally updated versions - target_sources supports using $<TARGET_OBJECTS:...> since CMake 3.5.0.

hintonda accepted this revision.Jun 20 2019, 7:32 AM

Finally updated versions - target_sources supports using $<TARGET_OBJECTS:...> since CMake 3.5.0.

Great, thanks for narrowing that down,

Still LGTM!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 8:03 AM

I really don't think this is the right solution.

$<TARGET_OBJECTS:lldb-mi-utils> can be passed in as source files in the unit test target, which should work just fine on CMake 3.4.x.

@beanz you're absolutely right, thank you.

This revision is now accepted and ready to land.Jun 21 2019, 4:40 AM
This revision was automatically updated to reflect the committed changes.