[PATCH] D15528: Teach clang-tidy how to -Werror checks.

[Jonathan Roelofs via cfe-commits]

jroelofs added a comment.

In http://reviews.llvm.org/D15528#311135, @alexfh wrote:

> In http://reviews.llvm.org/D15528#311053, @jroelofs wrote:
>
> > In http://reviews.llvm.org/D15528#311019, @alexfh wrote:
> >
> > > Jonathan, can you explain what specific use case does this patch address? Why one severity level of native clang-tidy warnings (the current situation) is not enough, and two levels are enough?
> >
> >
> > I have out-of-tree checkers for a very strange out-of-tree target. Some of the checks are on the level of "this should break the build because it cannot possibly work on this target" and others on the level of "tell me about it, but don't force me to fix it". All of these checks are things that don't even remotely apply to other targets.
>
>
> Thank you for the explanation. One more question: do you need to define Werrors differently in different directories?


Yeah, for things like "if you do this, you will get terribad performance", it's useful to be able to let the user's build system control them independently with that kind of granularity. In some places it's acceptable to fail these checks (i.e. while porting code to this target), and in others it's mission-critical.

I could see this being useful in other contexts too. For example, hypothetically, one could imagine turning on the llvm-include-order check for all of llvm, and then making it -Werror for subdirectories where it has been cleaned up, ensuring monotonic progress.

> 

> 

> > If you're wondering why I haven't hacked up Clang's sema to enforce these constraints, see again: out-of-tree backend... maintaining OOT changes there is expected to be very difficult.

> 

> 

> No, a sane person wouldn't suggest maintaining a local patch for Clang as a good solution ;)


:)

> 

> 

> > Clang-tidy however provides a very nice framework where they can be kept neatly off to the side, away from most of the merge hassles.

> 

> 

> It's one of the goals of clang-tidy to provide an easy way to maintain out-of-tree checks.


Seems to be working out very well on my end... thanks for designing it that way!

> 

> 

> > It'd be nice not to have to run clang-tidy twice & parse its output in order to achieve all of that, hence this patch.

> 

> 

> Agreed, I want to ensure though, that this is the right approach. In particular, I wonder whether a way to assign labels or severities to clang-tidy diagnostics would be better. Another question is whether we can reuse something from the Clang diagnostic subsystem to map warning levels.


For my purposes, this kind of trimodal { "it's definitely broken, and cannot possibly work", "hey, have you considered this?", "the tool didn't spot any problems" } granularity seems like the right fit, and lines up with Clang's diagnostics: notes usually stay notes, remarks stay remarks, warnings are sometimes errors and other times just warnings, and errors are errors.

Something else to think about: grouping a la -Wpedantic/-Wall/-Weverything might be useful. Some of that can be achieved by giving structure to the names of the checks, i.e.:

- footarget-perf-check1
- footarget-perf-check2
- footarget-correctness-check1
- footarget-correctness-check2
- footarget-portability-check1

  (pardon my vagueness here)

But that kind of breaks down when you start to talk about putting existing upstream checks into these groups, because then you have to write things like: `-checks='-*,footarget-portability-*,google-runtime-int-std' (off the cuff example). Not sure if that's what you meant by "labels", but I see that as being orthogonal to the question of what a user should be forced to take action on.


http://reviews.llvm.org/D15528