Page MenuHomePhabricator

[clang-tidy] Add options to bugprone-unused-return-value.
AbandonedPublic

Authored by balazske on Oct 6 2021, 6:24 AM.

Details

Summary

Extend the check with option to include or exclude specific functions.
This makes it more easy to add extra functions or remove from the checked function set
without specifying all functions.

Diff Detail

Event Timeline

balazske created this revision.Oct 6 2021, 6:24 AM
balazske requested review of this revision.Oct 6 2021, 6:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 6:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

These options are added to make the configuration more comfortable if only functions are to be added or removed from the list. I plan another commit that extends the list of checked functions significantly (to include all or some of the functions listed in CERT ERR33-C) (but after looking at D76083 I am not sure).

I think this is missing changes to the documentation as well.

These options are added to make the configuration more comfortable if only functions are to be added or removed from the list. I plan another commit that extends the list of checked functions significantly (to include all or some of the functions listed in CERT ERR33-C) (but after looking at D76083 I am not sure).

Personally, I think that would be very useful. One possible approach if there's concerns over adding that list in bugprone would be to add an alias to this check named cert-err33-c and have its default configuration include the listed functions.

clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
33

Any reason not to use StringRef here given that it's a constant list?

balazske updated this revision to Diff 378113.Oct 8 2021, 12:39 AM

Using StringRef, added documentation (untested).

Thank you for the new documentation, that helps! The one thing I'm still struggling with is the design approach and how well this will scale. Basically, the issue here is that CheckedFunctions already has a very long list of options, and you want to be able to manage that list (add entries to it or remove entries from it) without having to re-specify the entire list, correct? My concerns are: 0) It's a confusing design because the complete list is now spread over three different options that you have to think about, instead of just "here's the list of entries", 1) this problem can appear in *any* config list that gets large (for example, when you have a config file in a parent directory and a child directory and the parent adds a bunch of items to a list that the child wants to manage).

I wonder if we should be considering a more general approach like adding some set manipulation functionality to the config list itself. Thinking out loud (haven't put a ton of thought into this), we could devise some sort of syntax like:

{key: bugprone-unused-return-value.IncludedFunctions, value: "$add$($remove$(bugprone-unused-return-value.IncludedFunctions, "::excludedFun"), "::includedFun,::otherFun"},

That would remove ::excludedFun from the default list and add ::includedFun and ::otherFun to the list. Then this functionality could be used in any config setting that involves lists of elements rather than adding additional config options to each check. WDYT?

Probably it can work to only have a "add" or "remove" possibility in the config string, and not using an option name like in the provided example. Otherwise it can be possible to mix string lists from different checks, that seems to be a difficult problem. Other solution is to have a special list (or set) configuration format.

Probably it can work to only have a "add" or "remove" possibility in the config string, and not using an option name like in the provided example. Otherwise it can be possible to mix string lists from different checks, that seems to be a difficult problem. Other solution is to have a special list (or set) configuration format.

Hmm, I was thinking that was more of a feature than a problem. We have some checks that already have parallel lists of somewhat related strings, so being able to say "define this list in terms of that list plus/minus these elements" seems useful. Or are you saying there's an implementation difficulty with supporting that which might make it less of a good approach?

I think a configuration option that depends on configuration of another check causes too many problems. It causes dependencies between checks, possible circular dependency (that is an error). What if the other check is not enabled (if a check configuration depends on other it may mean that these two belong together and should both be enabled, or not)? It becomes less clear from where a value comes.

A probably better way: Define configuration values like "variables" that can be used at any check, probably with manipulation of the original value. But not reuse configuration of another check.

I think a configuration option that depends on configuration of another check causes too many problems. It causes dependencies between checks, possible circular dependency (that is an error). What if the other check is not enabled (if a check configuration depends on other it may mean that these two belong together and should both be enabled, or not)? It becomes less clear from where a value comes.

Okay, that make sense to me, thanks!

A probably better way: Define configuration values like "variables" that can be used at any check, probably with manipulation of the original value. But not reuse configuration of another check.

That might make more sense. Are we agreed that it would be better to solve the larger issue instead of just for this check?

balazske abandoned this revision.Oct 12 2021, 2:38 AM

Checker configuration can be solved in reusable way by introducing "configuration variables" and manipulation of these with set operations.

https://reviews.llvm.org/D111623 was created to update the checker documentation.