This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Enable the use of IgnoreArray flag in pro-type-member-init rule
ClosedPublic

Authored by h-joo on Apr 24 2021, 10:27 AM.

Details

Summary

The IgnoreArray flag was not used before while running the rule. Fixes b/47288

Diff Detail

Event Timeline

h-joo created this revision.Apr 24 2021, 10:27 AM
h-joo requested review of this revision.Apr 24 2021, 10:27 AM
h-joo retitled this revision from Enable IgnoreArray flag in pro-type-member-init rule to Enable the use of IgnoreArray flag in pro-type-member-init rule.Apr 24 2021, 10:38 AM
h-joo edited the summary of this revision. (Show Details)
njames93 retitled this revision from Enable the use of IgnoreArray flag in pro-type-member-init rule to [clang-tidy] Enable the use of IgnoreArray flag in pro-type-member-init rule.Apr 30 2021, 1:26 PM
njames93 added a subscriber: njames93.
njames93 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.ignorearrays.cpp
5

Small nit.

h-joo updated this revision to Diff 342233.May 2 2021, 4:32 AM

Change 1 to true in the test flag as requested by the reviewer.

h-joo marked an inline comment as done.May 2 2021, 4:32 AM

Whats the expected behaviour of sugar types. Can tests be added that demonstrate the behaviour.
I'd argue these cases shouldn't be warned on if IgnoreArrays is enabled and If that behaviour isn't observed currently it should be addressed.

typedef int TypedefArray[4];
using UsingArray = int[4];

struct HasArrayMember {
  HasArrayMember() {}
  UsingArray U; // Don't warn on this?
  TypedefArray T; // Or this?
  int Number; // Do warn this.
};
h-joo updated this revision to Diff 343864.May 8 2021, 2:00 PM

Extended the test case to confirm the fix also works for alias types(typedef, using).

h-joo added a comment.EditedMay 8 2021, 2:02 PM

Whats the expected behaviour of sugar types. Can tests be added that demonstrate the behaviour.
I'd argue these cases shouldn't be warned on if IgnoreArrays is enabled and If that behaviour isn't observed currently it should be addressed.

typedef int TypedefArray[4];
using UsingArray = int[4];

struct HasArrayMember {
  HasArrayMember() {}
  UsingArray U; // Don't warn on this?
  TypedefArray T; // Or this?
  int Number; // Do warn this.
};

Extended the existing test case to confirm that it works fine :)

njames93 accepted this revision.May 9 2021, 8:54 AM

LGTM, thanks.

This revision is now accepted and ready to land.May 9 2021, 8:54 AM
h-joo added a comment.May 9 2021, 9:41 AM

LGTM, thanks.

@njames93, I appreciate your time for the review. May I ask one more thing? I do not have commit rights. Would it be possible for you to make the commit? Thank you!!

LGTM, thanks.

@njames93, I appreciate your time for the review. May I ask one more thing? I do not have commit rights. Would it be possible for you to make the commit? Thank you!!

No problem, Can you please provide your GitHub username and email so I can mark you as the author of the patch.

h-joo added a comment.EditedMay 10 2021, 1:57 AM

LGTM, thanks.

@njames93, I appreciate your time for the review. May I ask one more thing? I do not have commit rights. Would it be possible for you to make the commit? Thank you!!

No problem, Can you please provide your GitHub username and email so I can mark you as the author of the patch.

I don't know which one you're asking for but if it's for the commit message git user.name Hana Joo and git user.email is hanajoo@google.com and my github username is h-joo same as the phabricator account. Thank you for your help!