Page MenuHomePhabricator

[clang-tidy] Add a new check group 'experimental-'
Changes PlannedPublic

Authored by whisperity on Mar 21 2020, 5:30 AM.

Details

Summary

As discussed in D69560 with @aaron.ballman, sometimes checks might not approximate the rule they target the best due to a variety of reasons. This new group should house checks that are working (i.e. no crashes and stupid outputs) but might not yet exactly match the rule we are trying to "sell them" as.

Diff Detail

Event Timeline

whisperity created this revision.Mar 21 2020, 5:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2020, 5:30 AM

I'm not sold on this being necessary as there is nothing against simply putting the check in the right module to begin with, then changing the registerCheck call to experimenal-<name> as well as the docs.
Could even do some fancy trickery with the python scripts to add support in there for it.
python add_new_check.py <module> <name> --experimental
python rename_check.py --no-experimental <check_name> // Removes a check from experimental when ready for gen use

I think we want to keep the experimental checks grouped to their parent module rather than being in a module of their own. Also, if we want to allow experimental checks, I think we should have some rough community consensus on 1) what are the criteria for something being experimental and included in clang-tidy (as opposed to in an out-of-tree repo) and what are the maintenance expectations for those checks, 2) what is the criteria for moving something *out* of experimental mode, and 3) how long should something be experimental without improvement before it's removed from clang-tidy?

I'm not opposed to the idea of experimental checks, but I don't want to see "experimental" become a dumping ground for low-quality checks that we then have to maintain indefinitely.

I think we want to keep the experimental checks grouped to their parent module rather than being in a module of their own.

For this, wouldn't the fact that telling Tidy to enable cppcoreguidelines-* (i.e. --checks='-*,cppcoreguidelines-*') also enables the cppcoreguidelines-experimental-* cause a problem? (This is the original reason why I tinkered in the "top-level" experimental- mode.)

I'm not opposed to the idea of experimental checks, but I don't want to see "experimental" become a dumping ground for low-quality checks that we then have to maintain indefinitely.

I think this could (in terms of development, support, etc.) work similarly to the alpha. group/package of Clang Static Analyser checkers. I'm not too knowledgeable about CSA - perhaps @Szelethus could help me out, - but their idea of alpha checkers is that they are mature in terms of the general idea, but clunky in terms of working. (The official stance is that anything out of alpha should not crash, anything within alpha might crash, and if you enable it and you crash, you are your own perpetrator.)

@aaron.ballman @njames93 Should I write up a pitch mail to cfe-dev about this?

I think we want to keep the experimental checks grouped to their parent module rather than being in a module of their own.

For this, wouldn't the fact that telling Tidy to enable cppcoreguidelines-* (i.e. --checks='-*,cppcoreguidelines-*') also enables the cppcoreguidelines-experimental-* cause a problem? (This is the original reason why I tinkered in the "top-level" experimental- mode.)

It will by default, but it doesn't have to. For instance, we could add an --experimental flag to clang-tidy that automatically enables the <module>-experimental-* checks in any otherwise-enabled module for the run.

I'm not opposed to the idea of experimental checks, but I don't want to see "experimental" become a dumping ground for low-quality checks that we then have to maintain indefinitely.

I think this could (in terms of development, support, etc.) work similarly to the alpha. group/package of Clang Static Analyser checkers. I'm not too knowledgeable about CSA - perhaps @Szelethus could help me out, - but their idea of alpha checkers is that they are mature in terms of the general idea, but clunky in terms of working. (The official stance is that anything out of alpha should not crash, anything within alpha might crash, and if you enable it and you crash, you are your own perpetrator.)

The alpha checks from CSA was the situation I was hoping to avoid -- my experience has been that checks go into there and rarely come out of alpha stage but continue to require periodic maintenance.

@aaron.ballman @njames93 Should I write up a pitch mail to cfe-dev about this?

I think that would be a reasonable next step.

whisperity planned changes to this revision.Mar 23 2020, 11:48 AM