This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add new readability non-idiomatic static access
ClosedPublic

Authored by barancsuk on Jul 27 2017, 4:41 AM.

Details

Summary

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;

Diff Detail

Event Timeline

barancsuk created this revision.Jul 27 2017, 4:41 AM
Eugene.Zelenko added inline comments.Jul 27 2017, 7:16 AM
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
7

Is :: operator really?

Eugene.Zelenko added a subscriber: Eugene.Zelenko.

It also seems that your code is not based on trunk, since Release Notes were cleared when version was changed to 6. Please update.

aaron.ballman added inline comments.Jul 27 2017, 7:40 AM
clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
23

Why not use isStaticStorageClass() here as well?

clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
20

:: isn't an operator -- I would say: "and replaces them with uses of the appropriate qualified-id."

docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
7

Same wording suggestion here as above.

test/clang-tidy/readability-static-accessed-through-instance.cpp
34

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.

137

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.

test/clang-tidy/readability-static-accessed-through-instance.cpp
34

Maybe the correct fix would be here f(1, 2, 3, 4); C::x;

xazax.hun added inline comments.Jul 28 2017, 12:54 AM
test/clang-tidy/readability-static-accessed-through-instance.cpp
34

Maybe for now we should just skip this cases. Expr::HasSideEffects might be a good candidate to filter these cases.

baloghadamsoftware added a comment.EditedJul 28 2017, 1:26 AM

I am not sure whether we should cope with such ugly things as overloaded member access operators with side effects, but they can also cause troubles using this fix:

#include <iostream>

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: "<<cp->n<<endl;
  cp->N = 10;
  cout<<"n: "<<cp->n<<endl;
}

Output:

n: 1
n: 3

After the fix, the output changes to:

n: 1
n: 2
test/clang-tidy/readability-static-accessed-through-instance.cpp
34

I think including the expression with side effect before the member access (as I suggested) is not more complicated than skipping these cases.

aaron.ballman added inline comments.Jul 28 2017, 5:10 AM
test/clang-tidy/readability-static-accessed-through-instance.cpp
34

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)
    ;
}
barancsuk updated this revision to Diff 109120.Aug 1 2017, 7:53 AM
barancsuk marked 7 inline comments as done.

Address review comments

barancsuk added inline comments.Aug 1 2017, 7:58 AM
clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
23

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.

docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
7

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.

test/clang-tidy/readability-static-accessed-through-instance.cpp
34

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.

aaron.ballman added inline comments.Aug 2 2017, 10:04 AM
clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
23

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?

31

No else after a return.

80

No need for the extra parens here.

clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
20

Reflow the comments.

test/clang-tidy/readability-static-accessed-through-instance.cpp
34

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.

91

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.

barancsuk updated this revision to Diff 110175.Aug 8 2017, 5:05 AM
barancsuk marked 4 inline comments as done.

Further reviews addressed

barancsuk added inline comments.Aug 8 2017, 5:52 AM
clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
23

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.

aaron.ballman accepted this revision.Aug 8 2017, 8:15 AM

LGTM!

clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
23

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.

This revision is now accepted and ready to land.Aug 8 2017, 8:15 AM
This revision was automatically updated to reflect the committed changes.
alexfh edited edge metadata.Aug 9 2017, 11:23 AM

A few late comments.

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
28–29 ↗(On Diff #110209)

This may be confusing as to whether the check removes the struct definition (and the definition of c1) or not.

docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
11

typo: "Thefollowing"

test/clang-tidy/readability-static-accessed-through-instance.cpp
55

Line wrapping gone bad. I guess you can just remove the second line. Same below.

barancsuk marked 3 inline comments as done.Aug 10 2017, 11:51 PM
barancsuk added inline comments.
test/clang-tidy/readability-static-accessed-through-instance.cpp
55

I think the line wrapping and the typo have already been corrected in a later version.

Please look at recently filed PR40544.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 11:03 AM