[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
vmiklos added a comment. Sorry, forgot to use the magic line at the end of the commit message to auto-close this review. Done in r351686, anyhow. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56966/new/ https://reviews.llvm.org/D56966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
lebedev.ri added inline comments. Comment at: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9 -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. +Finds classes that contain non-static data members in addition to user-provided +non-static member functions and diagnose all data members declared with a riccibruno wrote: > lebedev.ri wrote: > > riccibruno wrote: > > > 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. > > 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 > > [[ https://en.cppreference.com/w/cpp/language/explicit | explicit specifier > > ]] which is not what is meant. > 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" ? > 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56966/new/ https://reviews.llvm.org/D56966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
riccibruno added inline comments. Comment at: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9 -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. +Finds classes that contain non-static data members in addition to user-provided +non-static member functions and diagnose all data members declared with a lebedev.ri wrote: > riccibruno wrote: > > 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. > 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 [[ > https://en.cppreference.com/w/cpp/language/explicit | explicit specifier ]] > which is not what is meant. 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" ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56966/new/ https://reviews.llvm.org/D56966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
lebedev.ri added inline comments. Comment at: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9 -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. +Finds classes that contain non-static data members in addition to user-provided +non-static member functions and diagnose all data members declared with a riccibruno wrote: > 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. 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 [[ https://en.cppreference.com/w/cpp/language/explicit | explicit specifier ]] which is not what is meant. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56966/new/ https://reviews.llvm.org/D56966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.
LG other than two nits, thank you!
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:25
AST_MATCHER(CXXRecordDecl, hasNonStaticMethod) {
+ return hasMethod(unless(anyOf(isStaticStorageClass(), isImplicit(
Please do rename it though, from `hasNonStaticMethod` to
`hasNonStaticNonImplicitMethod` or something.
Comment at: test/clang-tidy/misc-non-private-member-variables-in-classes.cpp:38
+// Only data and implicit methods, do not warn
+
Can you please duplicate this test and add one static method (into
`S1Implicit`, i think?).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56966/new/
https://reviews.llvm.org/D56966
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
riccibruno added inline comments. Comment at: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9 -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. +Finds classes that contain non-static data members in addition to user-provided +non-static member functions and diagnose all data members declared with a 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56966/new/ https://reviews.llvm.org/D56966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
vmiklos updated this revision to Diff 182702.
vmiklos marked 3 inline comments as done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56966/new/
https://reviews.llvm.org/D56966
Files:
clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
Index: test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
===
--- test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
+++ test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
@@ -35,6 +35,18 @@
int S1_v3;
};
+// Only data and implicit methods, do not warn
+
+class C {
+public:
+ C() {}
+ ~C() {}
+};
+
+struct S1Implicit {
+ C S1Implicit_v0;
+};
+
////
// All functions are static, do not warn.
Index: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
===
--- docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
+++ docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
@@ -6,11 +6,11 @@
`cppcoreguidelines-non-private-member-variables-in-classes` redirects here
as an alias for this check.
-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.
+Finds classes that contain non-static data members in addition to user-provided
+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.
Options
---
Index: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
===
--- clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
+++ clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
@@ -23,7 +23,7 @@
}
AST_MATCHER(CXXRecordDecl, hasNonStaticMethod) {
- return hasMethod(unless(isStaticStorageClass()))
+ return hasMethod(unless(anyOf(isStaticStorageClass(), isImplicit(
.matches(Node, Finder, Builder);
}
@@ -66,8 +66,9 @@
IgnorePublicMemberVariables ? isProtected() : unless(isPrivate()));
// We only want the records that not only contain the mutable data
(non-static
- // member variables), but also have some logic (non-static member functions).
- // We may optionally ignore records where all the member variables are
public.
+ // member variables), but also have some logic (non-static, non-implicit
+ // member functions). We may optionally ignore records where all the member
+ // variables are public.
Finder->addMatcher(cxxRecordDecl(anyOf(isStruct(), isClass()), hasMethods(),
hasNonStaticMethod(),
unless(ShouldIgnoreRecord),
Index: test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
===
--- test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
+++ test/clang-tidy/misc-non-private-member-variables-in-classes.cpp
@@ -35,6 +35,18 @@
int S1_v3;
};
+// Only data and implicit methods, do not warn
+
+class C {
+public:
+ C() {}
+ ~C() {}
+};
+
+struct S1Implicit {
+ C S1Implicit_v0;
+};
+
////
// All functions are static, do not warn.
Index: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
===
--- docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
+++ docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst
@@ -6,11 +6,11 @@
`cppcoreguidelines-non-private-member-variables-in-classes` redirects here
as an alias for this check.
-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.
+Finds classes that contain non-static data members in addition to user-provided
+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
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
vmiklos marked 8 inline comments as done.
vmiklos added a comment.
I also noticed I forgot to clang-format the testcase, done now.
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:21
AST_MATCHER(CXXRecordDecl, hasMethods) {
+ for (const auto : Node.methods()) {
lebedev.ri wrote:
> let's keep this one as-is.
Done.
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:31-34
AST_MATCHER(CXXRecordDecl, hasNonStaticMethod) {
return hasMethod(unless(isStaticStorageClass()))
.matches(Node, Finder, Builder);
}
lebedev.ri wrote:
> And change this to something like
> ```
> AST_MATCHER(CXXRecordDecl, hasNonStaticNonImplicitMethod) {
> return hasMethod(unless(anyOf(isStaticStorageClass(), isImplicit(
> .matches(Node, Finder, Builder);
> }
> ```
Done.
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:74-76
// We only want the records that not only contain the mutable data
(non-static
// member variables), but also have some logic (non-static member functions).
// We may optionally ignore records where all the member variables are
public.
lebedev.ri wrote:
> Comment needs updating?
Done.
Comment at:
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9
-Finds classes that contain non-static data members in addition to non-static
+Finds classes that contain non-static data members in addition to explicit
non-static
member functions and diagnose all data members declared with a non-``public``
lebedev.ri wrote:
> 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.
OK, used "user-provided" instead.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56966/new/
https://reviews.llvm.org/D56966
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.
Some nits.
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:21
AST_MATCHER(CXXRecordDecl, hasMethods) {
+ for (const auto : Node.methods()) {
let's keep this one as-is.
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:31-34
AST_MATCHER(CXXRecordDecl, hasNonStaticMethod) {
return hasMethod(unless(isStaticStorageClass()))
.matches(Node, Finder, Builder);
}
And change this to something like
```
AST_MATCHER(CXXRecordDecl, hasNonStaticNonImplicitMethod) {
return hasMethod(unless(anyOf(isStaticStorageClass(), isImplicit(
.matches(Node, Finder, Builder);
}
```
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:74-76
// We only want the records that not only contain the mutable data
(non-static
// member variables), but also have some logic (non-static member functions).
// We may optionally ignore records where all the member variables are
public.
Comment needs updating?
Comment at:
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9
-Finds classes that contain non-static data members in addition to non-static
+Finds classes that contain non-static data members in addition to explicit
non-static
member functions and diagnose all data members declared with a non-``public``
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.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56966/new/
https://reviews.llvm.org/D56966
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods
JonasToth added inline comments.
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:22
AST_MATCHER(CXXRecordDecl, hasMethods) {
- return std::distance(Node.method_begin(), Node.method_end()) != 0;
+ for (const auto : Node.methods()) {
+if (Method->isImplicit())
maybe `return llvm::any_of(Node.methods(), [](const CXXMethodDecl /* dunno
which type this would be */& M) { return !M->isImplicit(); });`?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56966/new/
https://reviews.llvm.org/D56966
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
