[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
Eugene.Zelenko added a comment. Herald added subscribers: dkrupp, rnkovacs. Herald added a project: LLVM. Please look at recently filed PR40544. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D35937/new/ https://reviews.llvm.org/D35937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
barancsuk marked 3 inline comments as done.
barancsuk added inline comments.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:55
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
alexfh wrote:
> Line wrapping gone bad. I guess you can just remove the second line. Same
> below.
I think the line wrapping and the typo have already been corrected in a later
version.
Repository:
rL LLVM
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
alexfh added a comment.
A few late comments.
Comment at:
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-static-accessed-through-instance.rst:28-29
+
+ C::foo();
+ C::x;
+
This may be confusing as to whether the check removes the struct definition
(and the definition of c1) or not.
Comment at:
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst:11
+
+Thefollowing code:
+
typo: "Thefollowing"
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:55
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
Line wrapping gone bad. I guess you can just remove the second line. Same below.
Repository:
rL LLVM
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
This revision was automatically updated to reflect the committed changes.
Closed by commit rL310371: [clang-tidy] Add new readability non-idiomatic
static access check (authored by xazax).
Changed prior to commit:
https://reviews.llvm.org/D35937?vs=110175=110209#toc
Repository:
rL LLVM
https://reviews.llvm.org/D35937
Files:
clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance-nesting-threshold.cpp
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
Index: clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -0,0 +1,222 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+ static void foo();
+ static int x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+ void ns() const;
+};
+
+int C::x = 0;
+
+struct CC {
+ void foo();
+ int x;
+};
+
+template struct CT {
+ static T foo();
+ static T x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+};
+
+// Expressions with side effects
+C (int, int, int, int);
+void g() {
+ f(1, 2, 3, 4).x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} f(1, 2, 3, 4).x;{{$}}
+}
+
+int i(int &);
+void j(int);
+C h();
+bool a();
+int k(bool);
+
+void f(C c) {
+ j(i(h().x));
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: static member
+ // CHECK-FIXES: {{^}} j(i(h().x));{{$}}
+
+ // The execution of h() depends on the return value of a().
+ j(k(a() && h().x));
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static member
+ // CHECK-FIXES: {{^}} j(k(a() && h().x));{{$}}
+
+ if ([c]() {
+c.ns();
+return c;
+ }().x == 15)
+;
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: static member
+ // CHECK-FIXES: {{^}} if ([c]() {{{$}}
+}
+
+// Nested specifiers
+namespace N {
+struct V {
+ static int v;
+ struct T {
+static int t;
+struct U {
+ static int u;
+};
+ };
+};
+}
+
+void f(N::V::T::U u) {
+ N::V v;
+ v.v = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} N::V::v = 12;{{$}}
+
+ N::V::T w;
+ w.t = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} N::V::T::t = 12;{{$}}
+
+ // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+ u.u = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} u.u = 12;{{$}}
+
+ using B = N::V::T::U;
+ B b;
+ b.u;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} B::u;{{$}}
+}
+
+// Templates
+template T CT::x;
+
+template struct CCT {
+ T foo();
+ T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template void f(T t, C c) {
+ t.x; // OK, t is a template parameter.
+ c.x; // 1
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::x; // 1{{$}}
+}
+
+template struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template void h() {
+ S sN;
+ sN.x; // OK, value of N affects whether x is static or not.
+
+ S<2> s2;
+ s2.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+ C *c1 = new C();
+ c1->foo(); // 1
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::foo(); // 1{{$}}
+ c1->x; // 2
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::x; // 2{{$}}
+ c1->nsx; // OK, nsx is a non-static member.
+
+ const C *c2 = new C();
+ c2->foo(); // 2
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::foo(); // 2{{$}}
+
+ C::foo(); // OK, foo() is accessed using a
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23 + memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()), + varDecl(hasStaticStorageDuration(, + unless(isInTemplateInstantiation())) barancsuk wrote: > aaron.ballman wrote: > > barancsuk wrote: > > > aaron.ballman wrote: > > > > Why not use `isStaticStorageClass()` here as well? > > > Unfortunately, `isStaticStorageClass()` misses variable declarations that > > > do not contain the static keyword, including definitions of static > > > variables outside of their class. > > > However, `hasStaticStorageDuration()` has no problem finding all static > > > variable declarations correctly. > > Under what circumstances would that make a difference? If the variable is > > declared within the class with the static storage specifier, that > > declaration should be sufficient for the check, regardless of how the > > definition looks, no? > > > > If it does make a difference, can you add a test case that demonstrates > > that? > `isStaticStorageClass()` does fail for the current test cases. > > The reason for that is the function `hasDecl()`, that, for static variables > with multiple declarations, may return a declaration that does not contain > the `static` keyword. > For example, it finds `int C::x = 0;` rather than `static int x;` > Then `isStaticStorageClass()` discards it. > > However, `hasStaticStorageDuration()` works fine, because all declarations > are static storage duration, regardless of whether the `static` keyword is > present or not. Hmmm, that's unexpected (though not incorrect, from looking at the matcher implementations). Thank you for the explanation, I think it's fine for your patch. https://reviews.llvm.org/D35937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
barancsuk added inline comments. Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23 + memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()), + varDecl(hasStaticStorageDuration(, + unless(isInTemplateInstantiation())) aaron.ballman wrote: > barancsuk wrote: > > aaron.ballman wrote: > > > Why not use `isStaticStorageClass()` here as well? > > Unfortunately, `isStaticStorageClass()` misses variable declarations that > > do not contain the static keyword, including definitions of static > > variables outside of their class. > > However, `hasStaticStorageDuration()` has no problem finding all static > > variable declarations correctly. > Under what circumstances would that make a difference? If the variable is > declared within the class with the static storage specifier, that declaration > should be sufficient for the check, regardless of how the definition looks, > no? > > If it does make a difference, can you add a test case that demonstrates that? `isStaticStorageClass()` does fail for the current test cases. The reason for that is the function `hasDecl()`, that, for static variables with multiple declarations, may return a declaration that does not contain the `static` keyword. For example, it finds `int C::x = 0;` rather than `static int x;` Then `isStaticStorageClass()` discards it. However, `hasStaticStorageDuration()` works fine, because all declarations are static storage duration, regardless of whether the `static` keyword is present or not. https://reviews.llvm.org/D35937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
barancsuk updated this revision to Diff 110175.
barancsuk marked 4 inline comments as done.
barancsuk added a comment.
Further reviews addressed
https://reviews.llvm.org/D35937
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
test/clang-tidy/readability-static-accessed-through-instance-nesting-threshold.cpp
test/clang-tidy/readability-static-accessed-through-instance.cpp
Index: test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -0,0 +1,222 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+ static void foo();
+ static int x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+ void ns() const;
+};
+
+int C::x = 0;
+
+struct CC {
+ void foo();
+ int x;
+};
+
+template struct CT {
+ static T foo();
+ static T x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+};
+
+// Expressions with side effects
+C (int, int, int, int);
+void g() {
+ f(1, 2, 3, 4).x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} f(1, 2, 3, 4).x;{{$}}
+}
+
+int i(int &);
+void j(int);
+C h();
+bool a();
+int k(bool);
+
+void f(C c) {
+ j(i(h().x));
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: static member
+ // CHECK-FIXES: {{^}} j(i(h().x));{{$}}
+
+ // The execution of h() depends on the return value of a().
+ j(k(a() && h().x));
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static member
+ // CHECK-FIXES: {{^}} j(k(a() && h().x));{{$}}
+
+ if ([c]() {
+c.ns();
+return c;
+ }().x == 15)
+;
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: static member
+ // CHECK-FIXES: {{^}} if ([c]() {{{$}}
+}
+
+// Nested specifiers
+namespace N {
+struct V {
+ static int v;
+ struct T {
+static int t;
+struct U {
+ static int u;
+};
+ };
+};
+}
+
+void f(N::V::T::U u) {
+ N::V v;
+ v.v = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} N::V::v = 12;{{$}}
+
+ N::V::T w;
+ w.t = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} N::V::T::t = 12;{{$}}
+
+ // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+ u.u = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} u.u = 12;{{$}}
+
+ using B = N::V::T::U;
+ B b;
+ b.u;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} B::u;{{$}}
+}
+
+// Templates
+template T CT::x;
+
+template struct CCT {
+ T foo();
+ T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template void f(T t, C c) {
+ t.x; // OK, t is a template parameter.
+ c.x; // 1
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::x; // 1{{$}}
+}
+
+template struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template void h() {
+ S sN;
+ sN.x; // OK, value of N affects whether x is static or not.
+
+ S<2> s2;
+ s2.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+ C *c1 = new C();
+ c1->foo(); // 1
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::foo(); // 1{{$}}
+ c1->x; // 2
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::x; // 2{{$}}
+ c1->nsx; // OK, nsx is a non-static member.
+
+ const C *c2 = new C();
+ c2->foo(); // 2
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::foo(); // 2{{$}}
+
+ C::foo(); // OK, foo() is accessed using a qualified-id.
+ C::x; // OK, x is accessed using a qualified-id.
+
+ D d;
+ d.foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} D::foo();{{$}}
+ d.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} D::x;{{$}}
+
+ E e;
+ e.foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} E::foo();{{$}}
+ e.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ //
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+ memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+ varDecl(hasStaticStorageDuration(,
+ unless(isInTemplateInstantiation()))
barancsuk wrote:
> aaron.ballman wrote:
> > Why not use `isStaticStorageClass()` here as well?
> Unfortunately, `isStaticStorageClass()` misses variable declarations that do
> not contain the static keyword, including definitions of static variables
> outside of their class.
> However, `hasStaticStorageDuration()` has no problem finding all static
> variable declarations correctly.
Under what circumstances would that make a difference? If the variable is
declared within the class with the static storage specifier, that declaration
should be sufficient for the check, regardless of how the definition looks, no?
If it does make a difference, can you add a test case that demonstrates that?
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:30
+return NameSpecifierNestingLevel;
+ } else
+return 0;
No `else` after a `return`.
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:79
+ if (BaseExpr->HasSideEffects(*AstContext) ||
+ (getNameSpecifierNestingLevel(BaseType) > NameSpecifierNestingThreshold))
+return;
No need for the extra parens here.
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.h:20
+/// \@brief Checks for member expressions that access static members through
instances
+/// and replaces them with calls to the preferred, more readable `::` operator.
+///
aaron.ballman wrote:
> `::` isn't an operator -- I would say: "and replaces them with uses of the
> appropriate qualified-id."
Reflow the comments.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
barancsuk wrote:
> aaron.ballman wrote:
> > baloghadamsoftware wrote:
> > > xazax.hun wrote:
> > > > baloghadamsoftware wrote:
> > > > > aaron.ballman wrote:
> > > > > > This fix-it worries me because it changes the semantics of the
> > > > > > code. The function `f()` is no longer called, and so this isn't a
> > > > > > valid source transformation.
> > > > > Maybe the correct fix would be here f(1, 2, 3, 4); C::x;
> > > > Maybe for now we should just skip this cases. Expr::HasSideEffects
> > > > might be a good candidate to filter these cases.
> > > I think including the expression with side effect before the member
> > > access (as I suggested) is not more complicated than skipping these cases.
> > Please ensure you handle the more complex cases then, such as:
> > ```
> > struct S {
> > static int X;
> > };
> >
> > void g(int &);
> > S h();
> >
> > void f(S s) {
> > g(h().X);
> > if ([s]() { return s; }().X == 15)
> > ;
> > }
> > ```
> Expressions with side effects introduce some troublesome cases.
> For example, in the following code, the static member expression, `h().x`
> does not get evaluated if a() returns false.
>
> ```
> struct C {
> static int x;
> };
>
> void j(int);
> int k(bool);
> bool a();
> C h();
>
> void g(){
> j(k(a() && h().x));
> }
> ```
>
> Maybe for now, the check should filter these expressions with side effects.
> They might better be addressed in a follow-up patch.
> Maybe for now, the check should filter these expressions with side effects.
> They might better be addressed in a follow-up patch.
I think that's a good approach.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:90
+
+ // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+ u.u = 12;
It'd be good to have an additional RUN line that sets the nesting level to
something other than the default and ensure the behavior is consistent.
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
barancsuk added inline comments.
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+ memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+ varDecl(hasStaticStorageDuration(,
+ unless(isInTemplateInstantiation()))
aaron.ballman wrote:
> Why not use `isStaticStorageClass()` here as well?
Unfortunately, `isStaticStorageClass()` misses variable declarations that do
not contain the static keyword, including definitions of static variables
outside of their class.
However, `hasStaticStorageDuration()` has no problem finding all static
variable declarations correctly.
Comment at:
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst:7
+Checks for member expressions that access static members through instances, and
+replaces them with the corresponding expressions that use a more readable `::`
operator.
+
aaron.ballman wrote:
> Eugene.Zelenko wrote:
> > Is :: operator really?
> Same wording suggestion here as above.
The suggested wording is indeed a clearer one, and I rewrote the descriptions
and the documentation accordingly, however I found, that in the C++
International Standard `::` is referred to as a scope (resolution) operator.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
aaron.ballman wrote:
> baloghadamsoftware wrote:
> > xazax.hun wrote:
> > > baloghadamsoftware wrote:
> > > > aaron.ballman wrote:
> > > > > This fix-it worries me because it changes the semantics of the code.
> > > > > The function `f()` is no longer called, and so this isn't a valid
> > > > > source transformation.
> > > > Maybe the correct fix would be here f(1, 2, 3, 4); C::x;
> > > Maybe for now we should just skip this cases. Expr::HasSideEffects might
> > > be a good candidate to filter these cases.
> > I think including the expression with side effect before the member access
> > (as I suggested) is not more complicated than skipping these cases.
> Please ensure you handle the more complex cases then, such as:
> ```
> struct S {
> static int X;
> };
>
> void g(int &);
> S h();
>
> void f(S s) {
> g(h().X);
> if ([s]() { return s; }().X == 15)
> ;
> }
> ```
Expressions with side effects introduce some troublesome cases.
For example, in the following code, the static member expression, `h().x` does
not get evaluated if a() returns false.
```
struct C {
static int x;
};
void j(int);
int k(bool);
bool a();
C h();
void g(){
j(k(a() && h().x));
}
```
Maybe for now, the check should filter these expressions with side effects.
They might better be addressed in a follow-up patch.
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
barancsuk updated this revision to Diff 109120.
barancsuk marked 7 inline comments as done.
barancsuk added a comment.
Address review comments
https://reviews.llvm.org/D35937
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
test/clang-tidy/readability-static-accessed-through-instance.cpp
Index: test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -0,0 +1,222 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+ static void foo();
+ static int x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+ void ns() const;
+};
+
+int C::x = 0;
+
+struct CC {
+ void foo();
+ int x;
+};
+
+template struct CT {
+ static T foo();
+ static T x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+};
+
+// Expressions with side effects
+C (int, int, int, int);
+void g() {
+ f(1, 2, 3, 4).x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} f(1, 2, 3, 4).x;{{$}}
+}
+
+int i(int &);
+void j(int);
+C h();
+bool a();
+int k(bool);
+
+void f(C c) {
+ j(i(h().x));
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: static member
+ // CHECK-FIXES: {{^}} j(i(h().x));{{$}}
+
+ // The execution of h() depends on the return value of a().
+ j(k(a() && h().x));
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static member
+ // CHECK-FIXES: {{^}} j(k(a() && h().x));{{$}}
+
+ if ([c]() {
+c.ns();
+return c;
+ }().x == 15)
+;
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: static member
+ // CHECK-FIXES: {{^}} if ([c]() {{{$}}
+}
+
+// Nested specifiers
+namespace N {
+struct V {
+ static int v;
+ struct T {
+static int t;
+struct U {
+ static int u;
+};
+ };
+};
+}
+
+void f(N::V::T::U u) {
+ N::V v;
+ v.v = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} N::V::v = 12;{{$}}
+
+ N::V::T w;
+ w.t = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} N::V::T::t = 12;{{$}}
+
+ // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+ u.u = 12;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} u.u = 12;{{$}}
+
+ using B = N::V::T::U;
+ B b;
+ b.u;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} B::u;{{$}}
+}
+
+// Templates
+template T CT::x;
+
+template struct CCT {
+ T foo();
+ T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template void f(T t, C c) {
+ t.x; // OK, t is a template parameter.
+ c.x; // 1
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::x; // 1{{$}}
+}
+
+template struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template void h() {
+ S sN;
+ sN.x; // OK, value of N affects whether x is static or not.
+
+ S<2> s2;
+ s2.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+ C *c1 = new C();
+ c1->foo(); // 1
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::foo(); // 1{{$}}
+ c1->x; // 2
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::x; // 2{{$}}
+ c1->nsx; // OK, nsx is a non-static member.
+
+ const C *c2 = new C();
+ c2->foo(); // 2
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} C::foo(); // 2{{$}}
+
+ C::foo(); // OK, foo() is accessed using a qualified-id.
+ C::x; // OK, x is accessed using a qualified-id.
+
+ D d;
+ d.foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} D::foo();{{$}}
+ d.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} D::x;{{$}}
+
+ E e;
+ e.foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} E::foo();{{$}}
+ e.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+ // CHECK-FIXES: {{^}} E::x;{{$}}
+
+ CC *cc = new CC;
+
+ f(*c1, *c1);
+ f(*cc, *c1);
+
+
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
aaron.ballman added inline comments.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
baloghadamsoftware wrote:
> xazax.hun wrote:
> > baloghadamsoftware wrote:
> > > aaron.ballman wrote:
> > > > This fix-it worries me because it changes the semantics of the code.
> > > > The function `f()` is no longer called, and so this isn't a valid
> > > > source transformation.
> > > Maybe the correct fix would be here f(1, 2, 3, 4); C::x;
> > Maybe for now we should just skip this cases. Expr::HasSideEffects might be
> > a good candidate to filter these cases.
> I think including the expression with side effect before the member access
> (as I suggested) is not more complicated than skipping these cases.
Please ensure you handle the more complex cases then, such as:
```
struct S {
static int X;
};
void g(int &);
S h();
void f(S s) {
g(h().X);
if ([s]() { return s; }().X == 15)
;
}
```
Repository:
rL LLVM
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
baloghadamsoftware added inline comments.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
xazax.hun wrote:
> baloghadamsoftware wrote:
> > aaron.ballman wrote:
> > > This fix-it worries me because it changes the semantics of the code. The
> > > function `f()` is no longer called, and so this isn't a valid source
> > > transformation.
> > Maybe the correct fix would be here f(1, 2, 3, 4); C::x;
> Maybe for now we should just skip this cases. Expr::HasSideEffects might be a
> good candidate to filter these cases.
I think including the expression with side effect before the member access (as
I suggested) is not more complicated than skipping these cases.
Repository:
rL LLVM
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
baloghadamsoftware added a comment.
I am not sure whether we should copy with such ugly things as overloaded member
access operators with side effects, but they can also cause troubles using this
fix:
#include
using std::cout;
using std::endl;
struct C {
static int N;
int n = 0;
};
int C::N = 0;
struct Cptr {
C* c;
explicit Cptr(C *cc): c(cc) {}
C* operator->() {
++c->n;
return c;
}
};
int main() {
Cptr cp(new C);
cout<<"n: "N = 10;
cout<<"n: "
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
xazax.hun added inline comments.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
baloghadamsoftware wrote:
> aaron.ballman wrote:
> > This fix-it worries me because it changes the semantics of the code. The
> > function `f()` is no longer called, and so this isn't a valid source
> > transformation.
> Maybe the correct fix would be here f(1, 2, 3, 4); C::x;
Maybe for now we should just skip this cases. Expr::HasSideEffects might be a
good candidate to filter these cases.
Repository:
rL LLVM
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
baloghadamsoftware added inline comments.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
aaron.ballman wrote:
> This fix-it worries me because it changes the semantics of the code. The
> function `f()` is no longer called, and so this isn't a valid source
> transformation.
Maybe the correct fix would be here f(1, 2, 3, 4); C::x;
Repository:
rL LLVM
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+ memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+ varDecl(hasStaticStorageDuration(,
+ unless(isInTemplateInstantiation()))
Why not use `isStaticStorageClass()` here as well?
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.h:20
+/// \@brief Checks for member expressions that access static members through
instances
+/// and replaces them with calls to the preferred, more readable `::` operator.
+///
`::` isn't an operator -- I would say: "and replaces them with uses of the
appropriate qualified-id."
Comment at:
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst:7
+Checks for member expressions that access static members through instances, and
+replaces them with the corresponding expressions that use a more readable `::`
operator.
+
Eugene.Zelenko wrote:
> Is :: operator really?
Same wording suggestion here as above.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
This fix-it worries me because it changes the semantics of the code. The
function `f()` is no longer called, and so this isn't a valid source
transformation.
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:137
+ h<4>();
+}
Please add some tests where the replacement is somewhat more complex, like:
```
namespace N {
struct S {
struct T {
struct U {
static int x;
};
};
};
}
// Complex replacement
void f(N::S::T::U u) {
u.x = 12; // N::S::T::U::x = 12;
}
```
Note that this is a case where it's possible that the code becomes *less*
readable rather than more readable. I am fine without having any configuration
right now, but we may want to consider whether the nesting level should factor
in to whether we think this is more or less readable.
Repository:
rL LLVM
https://reviews.llvm.org/D35937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
Eugene.Zelenko added a comment. It also seems that your code is not based on trunk, since Release Notes were cleared when version was changed to 6. Please update. Repository: rL LLVM https://reviews.llvm.org/D35937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/readability-static-accessed-through-instance.rst:7 +Checks for member expressions that access static members through instances, and +replaces them with the corresponding expressions that use a more readable `::` operator. + Is :: operator really? Repository: rL LLVM https://reviews.llvm.org/D35937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access
barancsuk created this revision.
barancsuk added a project: clang-tools-extra.
Herald added subscribers: baloghadamsoftware, xazax.hun, whisperity,
JDevlieghere, mgorny.
Checks for member expressions that access static members through instances, and
replaces them with the corresponding expressions that use a more readable `::`
operator.
Example:
The following code:
struct C {
static void foo();
static int x;
};
C *c1=new C();
c1->foo();
c1->x;
is changed to:
C::foo();
C::x;
Repository:
rL LLVM
https://reviews.llvm.org/D35937
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
test/clang-tidy/readability-static-accessed-through-instance.cpp
Index: test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -0,0 +1,137 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+ static void foo();
+ static int x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using the scope operator.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+};
+
+struct CC {
+ void foo();
+ int x;
+};
+
+template struct CT {
+ static T foo();
+ static T x;
+ int nsx;
+ void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using the scope operator.
+foo(); // OK, foo() is accessed inside the struct.
+ }
+};
+
+C (int, int, int, int);
+void g() {
+ f(1, 2, 3, 4).x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
+
+template T CT::x;
+
+template struct CCT {
+ T foo();
+ T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template void f(T t, C c) {
+ t.x; // OK, t is a template parameter.
+ c.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+}
+
+template struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template void h() {
+ S sN;
+ sN.x; // OK, value of N affects whether x is static or not.
+
+ S<2> s2;
+ s2.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+ C *c1 = new C();
+ c1->foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::foo();{{$}}
+ c1->x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} C::x;{{$}}
+ c1->nsx; // OK, nsx is a non-static member.
+
+ C::foo(); // OK, foo() is accessed using the scope operator.
+ C::x; // OK, x is accessed using the scope operator.
+
+ D d;
+ d.foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} D::foo();{{$}}
+ d.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} D::x;{{$}}
+
+ E e;
+ e.foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} E::foo();{{$}}
+ e.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} E::x;{{$}}
+
+ CC *cc = new CC;
+
+ f(*c1, *c1);
+ f(*cc, *c1);
+
+ // Macros: OK, macros are not checked.
+ FOO((*c1));
+ X((*c1));
+ FOO((*cc));
+ X((*cc));
+
+ // Templates
+ CT ct;
+ ct.foo();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} CT::foo();{{$}}
+ ct.x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+ // instance [readability-static-accessed-through-instance]
+ // CHECK-FIXES: {{^}} CT::x;{{$}}
+ ct.nsx; // OK, nsx is a non-static member
+
+ CCT cct;
+ cct.foo(); // OK, CCT has no static members.
+
