[cfe-dev] [clang-tidy][RFC] Run each check only once

[Carlos Galvez via cfe-dev]

> Making it easier
for users to enable all of the checks at once requires check authors
to consider *all* interactions between modules, and to date that isn't
a price we've been willing to make check authors pay. Instead, we
typically view modules as being orthogonal sets of functionality that
may or may not play well together.

Fully agree to that, checks should absolutely be independent, granular and
focused on doing one single thing without depending on one another. And
they may conflict with each other. My point is that it should be *the
user* making
the decision as to how to handle conflicts, enabling and disabling
whichever checks they want. Why is that a problem? We are not requiring
extra work on LLVM developers. Why should they have a say on how the user
configures the tool? If developers have exposed this public CLI/config
interface, users are free to use it as they see fit (and pay the price of
their choices, of course).

> You're proposing to make aliases be sort of second-class checks

Hmm, not exactly, I agree with you that all publicly documented checks
should be regarded as "first-class" checks. What I'm proposing here could
be considered more as an "optimization" - don't run the same check twice,
just re-use previous results. If you know that you are going to run 2
checks that are identical, just run one of them.

> we've never really cared which module has the primary check and which one
has the alias check
I actually think this is one of the problems that you were concerned about
above. Today, not all checks are first-class. There are "primary" checks
and "alias" checks, and a decision has to be made as to where to put which,
which causes the very issues you are describing. Not all checks from
cppcoreguidelines are "primary", some come from "misc", so what? They
aren't any less valuable.

I refer to my question above - why do we even need this distinction between
"primary" and "alias", i.e. what problem is this solving? There's 3 checks
checking for C style arrays? That's great, why not just link them to the
same documentation, and internally (not leaked to the user) use the same
implementation?

The only use case I can think of to publicly document that there are
"alias" checks is to let the user know that they can "skip redundant
checks" by manually disabling them. If there's any other use case I'd be
very happy to know!

/Carlos



On Sun, Oct 3, 2021 at 6:19 PM Aaron Ballman <aaron at aaronballman.com> wrote:

> On Sun, Oct 3, 2021 at 11:24 AM Aaron Ballman <aaron at aaronballman.com>
> wrote:
> > To date, aliases have been surfaced to the user as a first-class check
> > (while the documentation typically redirects to the primary check for
> > simplicity of writing check aliases, there's no requirement that they
> > do so; other than that, aliases should behave the same as any other
> > check as far as the user is concerned.) You're proposing to make
> > aliases be sort of second-class checks that are more closely tied to
> > the primary check by providing an option that says aliases are
> > disabled as a blanket operation rather than a case-by-case basis. I am
> > not convinced this is a good approach. We may already have checks for
> > which this is a problem (where the aliased check and the primary check
> > are checking different things), and this closes off the design space
> > for such checks in the future (and possibly encourages less code reuse
> > in the process).
> >
> > Btw, I'm not saying "no way", I'm saying "let's make sure we're not
> > regressing functionality or painting ourselves into a corner."
>
> Also, to date I believe we've never really cared which module has the
> primary check and which one has the alias check (it's fine for the
> primary to be in cert- and the alias to be in bugprone- or vice
> versa), but with this sort of an option, that implementation detail is
> leaked out to the user in a more obvious way. If we go this route, we
> might have to move some checks around so there's some consistent rule
> to aid users (like, the general-purpose module holds the primary and
> the coding style guidelines get the alias), but it may not work in
> every situation (I can imagine a check living in two style guides but
> not a general purpose module, or two general purpose modules but not a
> style guide).
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211003/7bae9bee/attachment.html>