This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
ClosedPublic

Authored by vmiklos on Jan 19 2019, 12:49 PM.

Details

Summary

Otherwise we don't warn on a struct containing a single public int, but
we warn on a struct containing a single public std::string, which is
inconsistent.

Diff Detail

Event Timeline

vmiklos created this revision.Jan 19 2019, 12:49 PM
vmiklos set the repository for this revision to rCTE Clang Tools Extra.Jan 19 2019, 12:52 PM
JonasToth added inline comments.
clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
22

maybe return llvm::any_of(Node.methods(), [](const CXXMethodDecl /* dunno which type this would be */& M) { return !M->isImplicit(); });?

lebedev.ri requested changes to this revision.Jan 19 2019, 1:14 PM

Some nits.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
21

let's keep this one as-is.

31–34

And change this to something like

AST_MATCHER(CXXRecordDecl, hasNonStaticNonImplicitMethod) {
  return hasMethod(unless(anyOf(isStaticStorageClass(), isImplicit())))
      .matches(Node, Finder, Builder);
}
74–76

Comment needs updating?

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
9

I don't like the wording. We are not looking for explicit methods,
we are looking for user-provided methods, as opposed to compiler-provided methods.

This revision now requires changes to proceed.Jan 19 2019, 1:14 PM
vmiklos marked 8 inline comments as done.Jan 20 2019, 5:46 AM

I also noticed I forgot to clang-format the testcase, done now.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
21

Done.

31–34

Done.

74–76

Done.

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
9

OK, used "user-provided" instead.

vmiklos updated this revision to Diff 182702.Jan 20 2019, 5:46 AM
vmiklos marked 3 inline comments as done.
vmiklos marked an inline comment as done.
lebedev.ri accepted this revision.Jan 20 2019, 5:54 AM

LG other than two nits, thank you!

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
31

Please do rename it though, from hasNonStaticMethod to hasNonStaticNonImplicitMethod or something.

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
38

Can you please duplicate this test and add one static method (into S1Implicit, i think?).

This revision is now accepted and ready to land.Jan 20 2019, 5:54 AM
riccibruno added inline comments.
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
13

Just a small remark: What do you mean exactly by "user-provided" ?

The term "user-provided" is defined in 11.4.2 [dcl.fct.def.default]/5 as

A function is user-provided if it is user-declared and not explicitly
defaulted or deleted on its first declaration.

lebedev.ri added inline comments.Jan 20 2019, 5:56 AM
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
13

Yeah, i'm not sure what is the right word to use here,
suggestions welcomed.
The previous wording was confusing too, since non-implicit can be read as explicit specifier which is not what is meant.

riccibruno added inline comments.Jan 20 2019, 6:05 AM
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
13

A suggestion without looking at the rest of the patch:

It depends on whether you want to include explicitly defaulted/deleted
member functions. If yes, then use "user-declared" and otherwise
use "user-provided" ?

lebedev.ri added inline comments.Jan 20 2019, 6:12 AM
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
13

explicitly defaulted/deleted member functions.

Yes, i do not think those should be exempt, i.e. i think the current code is correct (more tests needed?)
So "user-declared" it is.

vmiklos marked 6 inline comments as done.Jan 20 2019, 6:28 AM

Sorry, forgot to use the magic line at the end of the commit message to auto-close this review. Done in r351686, anyhow.

vmiklos closed this revision.Jan 20 2019, 6:32 AM
MyDeveloperDay added a project: Restricted Project.Jan 20 2019, 7:42 AM