I recently got a failure with the autobuilder due to clang-tidy rejecting the name hash_value because it doesn’t follow mlir conventions. However, we have to name it this to make the ADL stuff in LLVM pick up the name. This caused a build failure.
I don’t think that clang-tidy is working well as an error in our post-submit tests. Can/should we turn this into a warning?
-Chris
I have encountered issues with clang-tidy as well: one of my patches and a similar patch by @stellaraccident ended up presenting as “failed” because it thought static methods defined in a C header were unused functions.
UPDATE: I wanted to clarify that these failures were in llvm and not circt
I’m curious about this specific error. Since we are adopting the same .clang-tidy config as upstream MLIR, I don’t know why it would fail for us and not them. I looked into an issue where clang-tidy barfed on our header guard in the lib/ directory, but didn’t have any issues with upstream MLIR. My hunch is it has to do with some paths hard coded into the checks that don’t work since we are outside the main LLVM source tree. I brought this up last week on the clang-tidy Discord… but haven’t heard back. If there is a way to solve this properly in clang-tidy (instead of NOLINT comments or other hacks), I’m happy to contribute.
More generally about the effectiveness of clang-tidy as an error in the automation: my two cents is if it doesn’t gate a PR from being merged or put a red X next to a commit, no one will look at the warnings. This is my experience from adding these sort of GitHub checks to other projects. Personally, as a newbie to C++ in general and the LLVM/MLIR styles specifically, I appreciate the guardrails. I know @jdd took care to ensure clang-tidy only runs on the code touched by a patch, so failures should only crop up when adding or refactoring code. This helps reduce a unnecessary lint noise while still making sure new contributions are within said guardrails.
I’m generally in favor of keeping clang-tidy in our GitHub workflow as-is, and addressing whatever makes it fail for us but not upstream MLIR. However, if others would prefer to just remove clang-tidy from being a blocker in GitHub, I’m not going to fight it.
It will fail the same way, and it’ll display a warning on Phabricator in presubmit that we just ignore.
There is no post-submit bot that runs clang-tidy and that would fail a build though.
I’m generally against producing warnings – they’re too easy to ignore. (Does anyone know if GitHub has support for yellow checks?) Also, a failed PR gate doesn’t prevent merging.
What I generally do is add //NOLINT or //NOLINTNEXTLINE for issues which have to violate some tidy check. This is annoying but some LLVM extension code (as you’ve encountered) violates MLIR tidy rules, which I think we should adhere to as much as possible while remaining tidy-clean.
More annoyingly, not all of the code in MLIR is tidy clean. Specifically, there’s no clang-tidy rule for constructing header guards not in an include/ directory so when you’ve got a non-public header in lib/ (for instance) that’s always a clang-tidy warning. @joker-eph, is there any way to fix this? We’ve run into it at least once in CIRCT.
What I do think is reasonable is to disable those checks in the master push builds.
Got it, thanks for the info.
Since this is what MLIR does, that sounds good to me.
I think not? Seems like there is a feature request for what we’d want that is getting no love: https://github.com/isaacs/github/issues/1465