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

[Don Hinton via Phabricator via llvm-commits]

hintonda marked an inline comment as done.
hintonda added a comment.

In D61660#1496266 <https://reviews.llvm.org/D61660#1496266>, @lebedev.ri wrote:

> I think i'm failing to convey the thoughts here.
>  Can you explain why you insist on having
>
>   if(COMMAND llvm_add_library)
>     function(benchmark_add_library target)
>       llvm_add_library(${target} ${ARGN})
>     endfunction()
>   else()
>     function(benchmark_add_library target)
>       add_library(${target} ${ARGN})
>     endfunction()
>   endif()
>
>
> in `llvm/utils/benchmark/CMakeLists.txt`?


Sorry, I seem to have touched a nerve, and certainly didn't mean to offend
anyone.  As a volunteer, my only goal is to do what I can to improve the
project.

So, here's my logic for structuring the patch in this way:

If the new function were used anywhere else in llvm, et al, other than
benchmark, I'd agree that it belongs AddLLVM.cmake.  However, as far as
I can see, it's only used in benchmark, so it made sense to isolate the
change and put the new function definitions where they are used.

Additionally, breaking up the if/else logic and putting part of it in llvm and the
other part in benchmark seems overly complicated.  It also unnecessarily
increases the technical debt in the llvm cmake files, and forces maintainers to
look in two files instead of one in order to grok what's going on.



================
Comment at: llvm/utils/benchmark/CMakeLists.txt:12
+
+  project (benchmark)
 endif()
----------------
beanz wrote:
> The `project` call isn't what resets the policy state. You actually need to pull `cmake_minimum_required` into the `if` block to prevent resetting the policy state:
> https://cmake.org/cmake/help/v3.0/command/cmake_minimum_required.html
Thanks for pointing that out.  Should `clang/CMakeLists.txt`, et al, be updated as well?  They don't include `cmake_minimum_required()` within the project conditional either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61660