This is an archive of the discontinued LLVM Phabricator instance.

Add a bugprone-argument-comment option: IgnoreSingleArgument.
ClosedPublic

Authored by xyb on Sep 1 2019, 5:08 PM.

Details

Summary

Add bugprone-argument-comment option: IgnoreSingleArgument.
When true, the check will ignore the single argument.

Sometimes, it's not necessary to add comment to single argument.
For example:

std::string name("Yubo Xie");
pScreen->SetWidth(1920);
pScreen->SetHeight(1080);

This option can ignore such single argument in bugprone-argument-comment check.

Diff Detail

Event Timeline

xyb created this revision.Sep 1 2019, 5:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2019, 5:08 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
alexfh accepted this revision.Sep 4 2019, 6:57 AM

LG with a comment.

clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
28

Why is it a global option? Are there other checks that would need the same option? The one below also needs to be check-local.

This revision is now accepted and ready to land.Sep 4 2019, 6:57 AM
xyb added a comment.Sep 4 2019, 7:13 AM

Thanks. BTW, I can't commit the patch by myself.

alexfh added a comment.Sep 4 2019, 8:52 AM
In D67056#1657540, @xyb wrote:

Thanks. BTW, I can't commit the patch by myself.

There's an outstanding comment. Please address it first, and I'll be happy to commit your patch.

xyb added inline comments.Sep 4 2019, 9:12 AM
clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
28

Sorry, I'm afraid I didn't get your point. Could you please explain more? This setting just follows the same pattern used for other settings. All other settings use "Options.getLocalOrGlobal(...)", I'm not sure why this setting need be different. Or do you mean we should change other settings ("StrictMode", "CommentBoolLiterals", "CommentIntegerLiterals", ...) to local options also?

alexfh added inline comments.Sep 4 2019, 9:46 AM
clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
28

Options are stored as a string -> string map. The key in this map is either the option name prepended with the check name (for check-local options) or just the option name (this way an option can be read by all checks). There are two ways to read options: OptionsView::get reads just the local name, OptionsView::getLocalOrGlobal tries the check-local name and then falls back to reading the option using its global name.

In this particular check only the StrictMode option makes sense to be global (a few other checks define an option with the same name and set of values, and it may make sense to configure a global default value). Other options are specific to this check and should be local.

xyb updated this revision to Diff 218741.Sep 4 2019, 10:22 AM
xyb marked 2 inline comments as done.
xyb added inline comments.
clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
28

Thanks for your explanation! Updated.

xyb updated this revision to Diff 218892.Sep 5 2019, 4:43 AM

Update patch.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2019, 7:48 AM