[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D116386#3226174 <https://reviews.llvm.org/D116386#3226174>, @LegalizeAdulthood wrote:

> Aaron, I think your comments are useful and I would be inclined to agree with you if I
> was the original author of this check.

Sorry, I hope I didn't give the impression I thought you had done anything wrong here, because you definitely haven't. I appreciate all of your efforts (and am glad to see you back in the community), and my resigning from the review is not indicative of me not wanting to collaborate with you! It's more a matter of: I've got ~50 code reviews in my queue as of this morning, on top of my own work, and I didn't want you to have to ping this for months on end only to never get feedback from me because C++ Core Guideline reviews always stay at the bottom of my queue due to the amount of effort involved in most of them.

> I treat the guidelines as just that: guidelines,
> not rules.  In the context of clang-tidy I think you're correct that some guidelines
> are easily turned into usable diagnostics and a subset of those can become enforceable
> rules with suggested fixits.
>
> In this case, the check only issues diagnostics, not fixits.  When the diagnostics result
> in many false positives (as per the open bug), then I think it's reasonable to narrow the
> scope of the check to omit the false positives.

This is not how clang-tidy typically handles coding guideline-specific rules. The general rule in clang-tidy is for checks based on coding guidelines to be configured to follow the guideline by default (with options to help tune things). If of sufficient value, we will add an alias check in `bugprone` (or another module that makes sense) and give it different default configuration settings than the guideline check, but users expect something that claims to check a guideline to actually check that guideline.

(The alternative is: we go back to the guideline authors and ask them to please improve their guidance and then we implement whatever the new rule says. But the end result is the same -- for checks in a module specific to a published set of guidelines, deviations from the spec are considered bugs to be fixed <somewhere>, and the guideline text is the final arbiter of what the correct behavior is.)

> The worst thing a "guideline" checker can do is to constantly nag you about false
> positives.  This trains people to not run the checkers and/or ignore all their complaints.

We're in strong agreement that guideline checkers with untenable false positive rates are a very bad thing. I think this particular check is of insufficient quality to have been added in the first place because it's based on a set of rules that are not generally enforceable in a way that isn't a constantly nagging for reasonable real world code.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116386/new/

https://reviews.llvm.org/D116386