[PATCH] D114712: [libc] Bazel overlay for libc

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey added a comment.

In D114712#3158597 <https://reviews.llvm.org/D114712#3158597>, @sivachandra wrote:

> Overall LGTM but one question: Will the file `libc/BUILD.bazel` interfere with downstream `BUILD` files?

I assume you're talking about Google's monorepo. No, it won't.



================
Comment at: utils/bazel/llvm-project-overlay/libc/fenv_targets.bzl:9
+
+def list_fenv_targets(name = None):
+    libc_function(
----------------
I'm confused why this would be pulled out into a macro rather than just defined as build rules directly in the BUILD.bazel file. This sort of indirection defeats tooling aimed at automated build file maintenance and makes it harder to inspect build files or understand where some target is defined. These macros also aren't really general purpose and require the other macros to have defined specific targets in order to function. I'm guessing the goal is to avoid a massive build file and organize things for readability. I think some of the typical reasons for factoring things are less applicable in build rules. As another idea, could you instead place a build file in src/fenv? It seems like all these rules use sources in src/fenv. This would be a much more natural way to factor things (similar for the other such macros). The only reason the LLVM and MLIR build files are one giant file is that the `lib/` and `include/` split make it difficult to factor things since Bazel defines packages by directories.

See, for reference https://docs.bazel.build/versions/main/skylark/bzl-style.html#macros


================
Comment at: utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl:11
+
+def libc_function(name, srcs, **kw):
+    """Add target for a libc function.
----------------
Please use `**kwargs`, which is used in all other macros in the project.


================
Comment at: utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl:26-29
+    if "deps" in kw:
+        kw["deps"].append(LIBC_ROOT_TARGET)
+    else:
+        kw["deps"] = [LIBC_ROOT_TARGET]
----------------
I think it would make more sense to make this a named kwarg in the function signature




================
Comment at: utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl:35
+    native.cc_library(
+        name=name + INTERNAL_SUFFIX,
+        srcs=srcs,
----------------
Normally, you'd use a `_` prefix for implementation-detail rules (as [recommended by the style guide](https://docs.bazel.build/versions/main/skylark/bzl-style.html#:~:text=All%20other%20targets%20defined%20by%20a%20macro%20should%20have%20their%20names%20preceded%20with%20an%20underscore))


================
Comment at: utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl:49
+
+def _libc_static_library_archive_impl(ctx):
+    toolchain = ctx.attr._cc_toolchain[cc_common.CcToolchainInfo]
----------------
Is this function used anywhere?


================
Comment at: utils/bazel/llvm-project-overlay/libc/math_targets.bzl:26
+    """
+    deps = [":__support_common"]
+    if need_fputil:
----------------
This macro will make understanding what srcs and deps a rule has more complicated. It may be worth the tradeoff, just consider https://docs.bazel.build/versions/main/skylark/bzl-style.html#macros when making this decision please


================
Comment at: utils/bazel/llvm-project-overlay/libc/math_targets.bzl:33-39
+    for s in specializations:
+        if s == "generic":
+            select_map["//conditions:default"] = ["src/math/generic/" + name + ".cpp"]
+        if s == "aarch64":
+            select_map["@platforms//cpu:arm64"] = ["src/math/aarch64/" + name + ".cpp"]
+        if s == "x86_64":
+            select_map["@platforms//cpu:x86_64"] = ["src/math/x86_64/" + name + ".cpp"]
----------------
I think this is equivalent, unless you want to use this opportunity to add error checking if a specialization is none of those things


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114712