[PATCH] D89142: llvmbuildectomy

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.
Herald added a subscriber: pengfei.

[style] The indentation convention for CMake files is two spaces.

In D89142#2364349 <https://reviews.llvm.org/D89142#2364349>, @serge-sans-paille wrote:

> That bein g said, I agree it's only the first step toward removing the LLVMBuild.* files, and the redunduncy between these files and the regular CMakeFiles.txt. I just don't want to do all the changes at once, and this one is already a big change!

It's still a detour. More work to get the intermediate state to a good state, its risk to break something, the confusion it creates to contributors. In case of Bazel, the intermediate state might also cause additional work (Bazel people may want to comment on that). Now, you already did the work with this patch (which is of high quality), but as a means to explore the end solution, we don't actually need to commit it.

It would be great to get some other opinions as well that see more value than I do.



================
Comment at: llvm/cmake/modules/LLVM-Build.cmake:17
+#
+function(LLVMBuildComponent)
+
----------------
serge-sans-paille wrote:
> Meinersbur wrote:
> > Even if already covered in `LLVMBuild.rst`, could you document which arguments are accepted here as well?
> I started doing that, but felt it's ultra-redundant. I'll reference the original doc to avoid duplication.
Like with doxygen comments, it's the first point of reference for the definition, independent of whether additional documentation exists. IMHO a short description is helpful.


================
Comment at: llvm/cmake/modules/LLVM-Build.cmake:176-177
+    set(LLVMBuildComponents ${llvmbuild_final_components} PARENT_SCOPE)
+
+
+endfunction()
----------------
[nit] surplus space


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

https://reviews.llvm.org/D89142