[PATCH] D143804: [bazel] create a clang-tidy binary target

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

GMNGeoffrey added inline comments.


================
Comment at: utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel:19
+# Include static analyzer checks in clang-tidy. Usage:
+#   $ bazel build -- at llvm-project//clang-tools-extra/clang-tidy:enable_static_analyzer=true //...
+#   $ bazel build -- at llvm-project//clang-tools-extra/clang-tidy:enable_static_analyzer=false //...
----------------
Note that we can also add a flag alias in the project bazelrc so that in-repo builds have a short way to spell this.


================
Comment at: utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel:67
+    hdrs = [":confusables_inc"],
+    include_prefix = ".",
+)
----------------
I think this unnecessary, right? That's the default. The headers should already be available at this path


================
Comment at: utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/defs.bzl:19
+def clang_tidy_library(name, **kwargs):
+    kwargs["srcs"] = kwargs.get("srcs", native.glob([paths.join(name, "*.cpp")]))
+    kwargs["hdrs"] = kwargs.get("hdrs", native.glob([paths.join(name, "*.h")]))
----------------
This saves a few keystrokes, but it adds a weird level of indirection here and a bunch of implicit behavior. Unfortunately, a lot of the literature on Bazel best practices I can only find internally, but generally it is recommend that you prioritize being DAMP rather than DRY. As a concerete example, does every one of these rules really depend on every one of these deps or is this adding unnecessary dependencies? I would actually recommend getting rid of this macro entirely and just expanding out its contents. In normal cases I'd even avoid globs for source files (outside of globbing tests), but given that Bazel is an unsupported build system for LLVM, we've compromised on using globs to reduce the frequency with which the BUILD.bazel files need to be updated.

See also, https://bazel.build/rules/bzl-style#macros


================
Comment at: utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/defs.bzl:24
+        name = name,
+        alwayslink = True,
+        **kwargs
----------------
Oof does it really have to be `alwayslink`? `alwayslink` creates a whole world of pain where things can fail do to changes far away from them in the build graph. What neds this behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143804