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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

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?

>> (2) I'm not as certain about -dump-config, since I am not really familiar with that option, but I think we want to list the alias and the original under that as well because different check names may have different configuration options.
> 
> It was because of -dump-config that I noticed the checks were running twice.
>  I had to configure a check in two places to be sure it was configured how I wanted.
>  I don't think there is a use case for running the same check with different options at the same time.

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.

> Dumping options that won't be used isn't good either.
> 
> I'd like to remove cert-oop11-cpp's UseCERTSemantics option - see my comment on https://reviews.llvm.org/D12839.

Removing that option may be possible so long as we still get the correct semantics when running cert-oop11-cpp.

> An alternative to clang-tidy preferring the original check would be to complain if both are enabled.

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.


https://reviews.llvm.org/D25659