[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 //...
----------------
aaronmondal wrote:
> GMNGeoffrey wrote:
> > 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.
> Yes, although I think we should try to not depend on the `.bazelrc` too much. If someone imports the project, the bazelrc is ignored (btw this also can cause builds to break already since we set the C++ standard in the .bazelrc and a naive workspace or module will try to build LLVM with an older, incompatible C++ standard).
> 
> We probably need "standardized" way to handle these config flags in general. If we move the entire LLVM config to build settings we'll have to handle like 50 config options. Improving the platforms may reduce that complexity a bit, but we'd probably still end up with quite a few manually tunable knobs.
Agreed we wouldn't want to *rely* on flag aliases, but it's also an advantage because it means we can choose an aliases that makes sense in the context of llvm, but would maybe be ambiguous in other projects. The alias is just a convenience, so I don't think that adding it is relying on it.

I never really found anyone who claimed that they could set up a Bazel toolchain though, which I think is the "proper" way to do this rather than the bazelrc configs. If someone is trying to build LLVM with an unsupported C++ version version though, that seems like on them? They need to have configured their C++ toolchain some way for the project they have that is presumably depending on LLVM, so will have chosen how to do that. We wouldn't want to mandate that LLVM has to be built with the One True Toolchain. It would be nice if the error message were friendlier though: just a quick failure saying "LLVM requires C++17" (are we on 17 as the minimum yet?). Not sure how to do that with Bazel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143804