This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix bugprone-argument-comment bug: negative literal number is not checked.
ClosedPublic

Authored by xyb on Sep 2 2019, 12:20 PM.

Diff Detail

Event Timeline

xyb created this revision.Sep 2 2019, 12:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2019, 12:20 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko retitled this revision from Fix bugprone-argument-comment bug: negative literal number is not checked. to [clang-tidy] Fix bugprone-argument-comment bug: negative literal number is not checked..Sep 2 2019, 9:54 PM
Eugene.Zelenko added a project: Restricted Project.
aaron.ballman added inline comments.Sep 3 2019, 1:00 PM
clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
240

The bug claims that this is only for handling negative literals, but this allows any unary operator. Is that intentional? If we're going to allow arbitrary unary operators, why not arbitrary binary operators as well?

xyb added inline comments.Sep 4 2019, 6:37 AM
clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
240

Actually, it handles "UnaryOperator Literal". So it will handle "+12", "-12", "~12". I can restrict it for UO_MINUS only, but is it real necessary?

xyb updated this revision to Diff 218683.Sep 4 2019, 6:58 AM

Update with full diff.

alexfh accepted this revision.Sep 4 2019, 7:01 AM

LG if there are no other concerns.

This revision is now accepted and ready to land.Sep 4 2019, 7:01 AM
aaron.ballman accepted this revision.Sep 4 2019, 7:03 AM

LGTM!

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

It's not that it's necessary, it's that I'm wondering why we think -12 is special but 0 - 12 is not, but that can be done incrementally later.

xyb added a comment.Sep 4 2019, 7:10 AM

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

alexfh added a comment.Sep 4 2019, 9:49 AM
In D67084#1657534, @xyb wrote:

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

The patch doesn't apply cleanly. Please rebase it against current HEAD.

xyb updated this revision to Diff 218893.Sep 5 2019, 4:57 AM

Rebase patch with current HEAD.

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