Page MenuHomePhabricator

[clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)
ClosedPublic

Authored by lebedev.ri on Oct 2 2018, 12:37 AM.

Details

Summary

Finds classes that not only contain the data (non-static member variables),
but also have logic (non-static member functions), and diagnoses all member
variables that have any other scope other than private. They should be
made private, and manipulated exclusively via the member functions.

Optionally, classes with all member variables being public could be
ignored, and optionally all public member variables could be ignored.

Options

  • IgnoreClassesWithAllMemberVariablesBeingPublic

    Allows to completely ignore classes if all the member variables in that class have public visibility.
  • IgnorePublicMemberVariables

    Allows to ignore (not diagnose) all the member variables with public visibility scope.

References:

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Oct 2 2018, 12:37 AM
lebedev.ri edited the summary of this revision. (Show Details)Oct 3 2018, 2:53 AM
lebedev.ri added a project: Restricted Project.
lebedev.ri updated this revision to Diff 168088.Oct 3 2018, 2:54 AM
lebedev.ri added a subscriber: cfe-commits.

Finds classes that not only contain the data (non-static member variables),
but also have logic (non-static member functions), and diagnoses all member
variables that have any other scope other than private. They should be
made private, and manipulated exclusively via the member functions.

Optionally, classes with all member variables being public could be
ignored, and optionally all public member variables could be ignored.

Options

  • IgnoreClassesWithAllMemberVariablesBeingPublic

    Allows to completely ignore classes if all the member variables in that class have public visibility.
  • IgnorePublicMemberVariables

    Allows to ignore (not diagnose) all the member variables with public visibility scope.

References:

lebedev.ri retitled this revision from [private][clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP) to [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP).Oct 3 2018, 2:57 AM
JonasToth added inline comments.Oct 3 2018, 3:10 AM
clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
26 ↗(On Diff #167886)

I think this and the next matcher can be a normal variable.

auto HasNonStaticMethod = hasMethod(unless(isStaticStorageClass()));

...
... allOf(anyOf(isStruct(), isClass()), HasMethods, HasNonStaticMethod)
...
55 ↗(On Diff #167886)

Please make this variable uppercase, same for next matcher

76 ↗(On Diff #167886)

Please add assertions that these pointers are not-null. Some random bug could match only one of these too and result in null-deref

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h
31 ↗(On Diff #167886)

i believe usually the private state is written at the bottom of a class. I dont have a strong opinion, but i believe alex does prefer it.

docs/ReleaseNotes.rst
99 ↗(On Diff #167886)

Please shorten the text in the release notes a bit. Usually one sentence should be enough, you can explain more in the docs

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
1 ↗(On Diff #167886)

I would prefer multiple test-files instead of mixing them all together.

9 ↗(On Diff #167886)

The test miss

  • templated classes (should be diagnosed normally)
  • inheritance (public, private, protected) (should not add a new warning in the subclass)
  • and macros creating members. (dont know what to do there, but just warn?)
lebedev.ri updated this revision to Diff 168162.Oct 3 2018, 1:30 PM
lebedev.ri marked 5 inline comments as done.

Address review notes - more tests.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
26 ↗(On Diff #167886)

They could, but these have a very meaningful meaning, so i'd argue it would not be worse to keep them
out here refactored as proper matchers, in case someone else needs them elsewhere (so he could just
move them from here into more general place, as opposed to writing a duplicating matcher.)

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
1 ↗(On Diff #167886)

Hmm, no. Then the tests will have to be duplicated in N files,
because i really do want to see what each of these 4 configurations do on the *same* input.

It only looks ugly because the script only supports -check-suffix=,
and not -check-suffixes=, which would significantly cut down on check-lines.

I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you have thoughts on how to avoid that? The C++ Core Guidelines seem silent on the matter -- this might be worth raising with the authors.

The HIC++ standard has an exception that requires the data type to be in an extern "C" context, but I don't see that reflected in the tests or code.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
22 ↗(On Diff #168162)

Can drop the clang:: qualifiers -- you're in the namespace clang scope already.

56 ↗(On Diff #168162)

Neither the C++ Core Guidelines nor HIC++ carve out an exception for union types. I think you should talk to the guideline authors to see what they think. Perhaps first try to diagnose unions the same as structs and classes and see how the results look to you -- if they look bad, you can incorporate those as examples when discussing with the authors.

82 ↗(On Diff #168162)

Drop the single quotes above and just pass in Field and Record directly -- the diagnostic printer will do the right thing.

JonasToth added inline comments.Oct 5 2018, 6:49 AM
clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
26 ↗(On Diff #167886)

Ok.

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
1 ↗(On Diff #167886)

Ok, if you prefer this.

I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you have thoughts on how to avoid that? The C++ Core Guidelines seem silent on the matter -- this might be worth raising with the authors.

I have an idea to avoid trivial set/get methods: Flag public data if it is modified internally. This could serve as a heuristic for pre/postconditions on that variable.
In general one could resolve all data-dependencies inside the class and figure out if a private variable would need to change if a public variable got changed. But that is way more complex!

Still worth asking the authors.

I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you have thoughts on how to avoid that? The C++ Core Guidelines seem silent on the matter -- this might be worth raising with the authors.

I have an idea to avoid trivial set/get methods: Flag public data if it is modified internally. This could serve as a heuristic for pre/postconditions on that variable.
In general one could resolve all data-dependencies inside the class and figure out if a private variable would need to change if a public variable got changed. But that is way more complex!

Hmm, I don't know that it would help with code like this:

class Base {
  int f; // Suggested by C.133/C.9

protected:
//  int f; // Disallowed by C.133

  int getF() const { return f; } // Suggested by C.133, disallowed by C.131
  void setF(int NewF) { f = NewF; }  // Suggested by C.133, disallowed by C.131
};

Still worth asking the authors.

C.131 seems to imply a minimal amount of trivial getters/setters before
diagnosing.

I feel that the CPPCG just want to force the programmer to think twice
instead of forbidding it totally.
It might be worth to have a more chatty/specific check for the CPPCG and
a strict "Don't do it" for HICPP.

From my experience with asking the CPPCG authors it takes a while for a
response and that in the end is
not necessarily telling which way to go. Experience might differ
depending on topic though.

Hmm, I don't know that it would help with code like this:

class Base {
  int f; // Suggested by C.133/C.9

protected:
//  int f; // Disallowed by C.133

  int getF() const { return f; } // Suggested by C.133, disallowed by C.131
  void setF(int NewF) { f = NewF; }  // Suggested by C.133, disallowed by C.131
};

Still worth asking the authors.

zinovy.nis added inline comments.
test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
1 ↗(On Diff #167886)

Not an issue anymore. You can use -check-suffixes since now.

lebedev.ri marked 2 inline comments as done.
lebedev.ri edited the summary of this revision. (Show Details)

Let's try to wrap this up.

FIXME: IgnoreClassesWithAllMemberVariablesBeingPublic needs to be somehow enabled for cppcoreguidelines check.
I don't know if it is possible, and how.

Anything i missed?

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
56 ↗(On Diff #168162)

As discussed, let's just drop HICPP alias,
and CPPCG only talks about struct/class, not union.
https://github.com/isocpp/CppCoreGuidelines/issues/1281

82 ↗(On Diff #168162)

Nice, but that does not seem to dump getKindName().

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
1 ↗(On Diff #167886)

Yay, look at this beauty :)

Dead code elimination.

JonasToth added inline comments.Oct 12 2018, 8:11 AM
test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
1 ↗(On Diff #167886)
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
113 ↗(On Diff #169405)

Please move new alias after new checks.

lebedev.ri marked an inline comment as done.Oct 12 2018, 11:28 PM
lebedev.ri added inline comments.
docs/ReleaseNotes.rst
113 ↗(On Diff #169405)

You mean after all the new checks, at the very bottom?
Because right now it is right after the check itself.

JonasToth added inline comments.Oct 13 2018, 1:50 AM
docs/ReleaseNotes.rst
113 ↗(On Diff #169405)

Yup, the aliases were mentioned last in the release notes (at least last release).

Sort release notes.

docs/ReleaseNotes.rst
113 ↗(On Diff #169405)

Seems rather arbitrarily and undocumented but whatever.

FIXME: IgnoreClassesWithAllMemberVariablesBeingPublic needs to be somehow enabled for cppcoreguidelines check.
I don't know if it is possible, and how.

IIRC, the idea is to override getModuleOptions() and specify different default configuration options for the alias.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
82 ↗(On Diff #168162)

Correct - it only works for automatically quoting NamedDecl objects

90 ↗(On Diff #169555)

I cannot think of a situation in which the class name is actually useful information to present to the user, so I'd recommend dropping it (and the kind name as well). However, if you have a specific case in mind where the class name is useful for understanding how to fix the issue, I'd love to see it.

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
9 ↗(On Diff #169555)

I'd reword this a bit to:

Finds classes that contain non-static data members in addition to non-static member functions and diagnose all data members declared with a non-``public`` access specifier. The data members should be declared as ``private`` and accessed through member functions instead of exposed to derived classes or class consumers.
14–15 ↗(On Diff #169555)

I'd drop this paragraph entirely.

28 ↗(On Diff #169555)

with public visibility scope -> declared with a public access specifier

lebedev.ri marked 6 inline comments as done.
  • Address nits
  • Proper default for CPPCG
  • Don't show record name. I indeed don't have a strong case here, it seems to be more useful to dump that info, but maybe not? ¯\_(ツ)_/¯
aaron.ballman accepted this revision.Oct 18 2018, 6:34 AM

LGTM aside from a minor nit.

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
84 ↗(On Diff #170065)

Don't use auto here.

This revision is now accepted and ready to land.Oct 18 2018, 6:34 AM

LGTM aside from a minor nit.

Thank you for the review!

lebedev.ri marked an inline comment as done.

Don't use auto in getModuleOptions().

Rebased to fix a merge conflict in release notes.

This revision was automatically updated to reflect the committed changes.