[PATCH] D25659: [clang-tidy] Avoid running aliased checks twice

[Malcolm Parsons via cfe-commits]

malcolm.parsons added a comment.

In https://reviews.llvm.org/D25659#585234, @aaron.ballman wrote:

> In https://reviews.llvm.org/D25659#585154, @malcolm.parsons wrote:
>
> > In https://reviews.llvm.org/D25659#584986, @aaron.ballman wrote:
> >
> > > (1) I think that the aliases and the originals should be listed with -list-checks, because these are names under which the checks may be run (and sometimes the name may even imply different semantics for the check).
> >
> >
> > I've always found it odd that -list-checks only lists the enabled checks.
> >  I think a list of enabled checks shouldn't report aliases that will not be used.
>
>
> Okay, that's a good point, I forgot that -list-checks only lists the enabled checks, not all of them. With that in mind, here's what I would expect to see:
>
> (0) -checks=* -list-checks
>  (1) -checks=-*, cert-* -list-checks
>  (2) -checks=*, -cert-* -list checks
>
> In (0), I would expect all checks to be listed, even aliases. Aliases may have semantic distinctions from other checks and the user said "run everything." However, for aliases which have no semantic distinction, there's no point to running the check twice even if the user said to run everything. Leaving aliases in or out results in an unsatisfying situation either way, unless we know whether an alias supplies different option values.
>  In (1), I would expect only the cert checks to be listed, even if they alias to something outside of the cert module. It would be strange to use those options and see something outside of the cert module listed, even if it had no semantic distinction, just because of cert rules that are an alias to something else.
>  In (2), I would expect to see every check *except* cert checks listed, regardless of aliasing in or out of the cert module. Similar rationale as (1).
>
> If I understand the output from this patch, only (0) is modified to not list the alias name because aliases are never enabled with -checks=* in this patch, but (1) and (2) do not change behavior?


0) doesn't list aliases.  I'd like a -list-all-checks option.

1. lists all the cert checks, as they aren't aliases for other cert checks.
2. doesn't list any cert checks, as none are enabled.

> Does -dump-config only list enabled checks as well, or does it list the configuration for all checks? If it's all checks, then the alias should be present because it may be configured differently. If it's only enabled checks, then I agree that it should only list what is being used.

The config is only dumped for the enabled checks.

> I don't think that preferring the original check is bad so long as we get the semantic options correct; the ability for an alias to be defined as "that check with these options being set this way" is important.

That might mean preferring the alias.  But there might be more than one.


https://reviews.llvm.org/D25659