[PATCH] D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree

[Don Hinton via Phabricator via llvm-commits]

hintonda added a comment.

In D61660#1496888 <https://reviews.llvm.org/D61660#1496888>, @beanz wrote:

> I think the two of you are speaking past each other with different goals.
>
> @lebedev.ri wants to minimize changes to google benchmark and is trying to steer this patch so that parts of it can be upstreamed back to Google. While that isn't wholly unreasonable, it also isn't LLVM contributor burden to upstream contributions to Google. Personally, I think the goal should be less about making patches upstream-able, and more about making them less likely to conflict with merges from upstream.
>
> @hintonda Is trying to minimize unnecessary changes to LLVM and encapsulating the necessary change to only where it needs to be, which is also reasonable.
>
> I think it is probably reasonable to define `benchmark_add_library` in `AddLLVM.cmake` so that the benchmark sources don't need to directly reference LLVM. You can unconditionally define the function in `AddLLVM.cmake` then you only have one side of the if statement in the benchmark library (checking if `benchmark_add_library`) is already defined. I think that solution will satisfy @lebedev.ri's goals.
>
> I can understand your desire to have the definition of `benchmark_add_library` in one place, but frankly CMake lacks any reasonable or reliable developer tools. If I see a call to `benchmark_add_library` I have to grep the entire CMake codebase to find possible definitions because there isn't reliable "jump to..." tooling. We also use a similar pattern to this in the runtime libraries with the `runtime_register_component` function, and it works well in practice.


Okay, that sounds reasonable.

Btw, I looked at the implementation of `llvm_add_library` and the wrapper will need to be a macro instead of a function so as not to break anything on Window -- there's at least one `set()` command that includes `PARENT_SCOPE`, which will break if we create another scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61660