Page MenuHomePhabricator

[clang-tidy] initial version of readability-convert-member-functions-to-static
ClosedPublic

Authored by mgehre on May 9 2019, 12:06 PM.

Details

Summary

Finds non-static member functions that can be made const or static.
The check conservatively tries to preserve logical costness in favor of
physical costness. It will not recommend to make member functions const
that call (const) member functions on a non-static data members,
e.g. std::unique_ptr::get because that might indicate logical
non-costness. Currently, it will only make member functions const if they
only access to this is to read members of builtin type.

I have run this check (repeatedly) over llvm-project. It made 1708 member functions
static. Out of those, I had to exclude 22 via NOLINT because their address
was taken and stored in a variable of pointer-to-member type (e.g. passed to
llvm::StringSwitch).
It also made 243 member functions const. (This is currently very conservative
to have no false-positives and can hopefully be extended in the future.)

You can find the results here: https://github.com/mgehre/llvm-project/commits/static_const_eval

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2019, 12:06 PM

@JonasToth tried to implement const check, so probably const and static part should be split. It's hard to tell about state of existing implementation, but you could continue it or at least borrow ideas and tests.

clang-tools-extra/clang-tidy/readability/StaticConstMethodCheck.cpp
33 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

101 ↗(On Diff #198882)

Please elide braces.

103 ↗(On Diff #198882)

You could return {}

124 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

129 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

134 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

183 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

199 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

213 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

226 ↗(On Diff #198882)

Unnecessary empty line.

228 ↗(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

clang-tools-extra/docs/ReleaseNotes.rst
72 ↗(On Diff #198882)

Please rebase from trunk.

Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko removed a subscriber: JonasToth.
lebedev.ri edited the summary of this revision. (Show Details)May 10 2019, 10:48 AM
lebedev.ri edited the summary of this revision. (Show Details)
lebedev.ri added a subscriber: lebedev.ri.

@JonasToth tried to implement const check, so probably const and static part should be split.

I agree, there is D54943, which should be 99% ready to go, and is only stuck due to @JonasToth
time constraints, IIRC. Treating *this pointer should simply be a logical extension of
that check. (unless it already does that, or there is some other patch i forgot about)

I.e. let's split this, and start with static part.

Also, you are aware of ExprMutationAnalyzer?

It's hard to tell about state of existing implementation, but you could continue it or at least borrow ideas and tests.

I have thought about splitting the check into separate static & const, but kept it together because
the analysis for both cases is very similar, i.e. finding the usage of this.

Also, both checks share many preconditions, e.g. not applying them to virtual methods or methods
on class with a template base class, not applying them to constructor, not applying them to operators, etc
(and those preconditons are not shared with D54943).

I had a look at D54943. Nice change! It will be very valuable to have the const analysis for variables!
Note, though, that the hard part of of making methods
const is not the detailed usage analysis as its done by the ExprMutationAnalyzer. Instead, the hard part
is to avoid false-positives that make the check unusable.
I had a version of this check that made a very detailed analysis just like ExprMutationAnalyzer
(restricted to this). But then most of the functions it flagged as const
(technically correct) on the LLVM code base we would not want to make const (logically). Examples:

class S {
  // Technically const (the pointer Pimp itself is not modified), but logically should not be const
  //  because we want to consider *Pimp as part of the data.
  void setFlag() {
    *Pimp = 4;
  }

  int* Pimp;
};
class S {
  void modify() {
     // Technically const because we call a const method
    // "pointer unique_ptr::operator->() const;"
    // but logically not const.
    U->modify();
  }

  std::unique_ptr<Object> U;
};
// Real-world code, see clang::ObjCInterfaceDecl.
class DataPattern {
  int& data() const;
public:

  int& get() { // Must not be const, even though it only calls const methods
    return data();
  }

  const int& get() const {
    return const_cast<DataPattern *>(this)->get();
  }

  void set() { // Must not be const, even though it only calls const methods
    data() = 42;
  }
};

and many more.

So the hard thing for the const check was to actually remove the sophisticated analysis I had, and come up with
something much simpler that would have no false positive (otherwise users will just disable the check) while
still finding something. I fear that the same could happen when we apply D54943 to this directly. It might
be easier to keep those two cases separate.

...

Still, this is two separate checks, logically.
You can put the FindUsageOfThis e.g. into a new file in clang-tidy/utils.

Hey, I do like your check!

I think it would be great to implement this analysis in utils/ as roman suggested. Then we have bigger flexibility.
The ExprMutAnalyzer was requested to be moved to clang/Analysis/ as the CSA folks wanted to reuse it at some point. That might be the case with your logic as well, as it is still related to ExprMutAnalyzer even though its not the same.

Another issue why the clang-tidy part for the variable const-suggestions doesn't move forward is that ExprMutAnalyzer itself is blocked due to @shuaiwang not having time as well. We have a view nasty cases that produce false positives and especially pointer analysis is complicated.

For that I think that @mgehre approach of producing something useful now should not be blocked on us. Especially since its unclear when we can continue our work substantially. Preparing for joining forces later would be great though! I think this is possible with having just a class/set of functions for the analysis that can be used in different parts of clang tools is sufficient for that.

JonasToth added inline comments.May 11 2019, 3:04 AM
clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp
209 ↗(On Diff #198882)

Does it pass here?
I looks a bit weird, shouldnt the lambda be called for this to work (this is the unary + right?)?

312 ↗(On Diff #198882)

I think more template tests wouldn't hurt. From the other checks experience I can safely say we got burned a few times :)
Instantiating the templates with builtin arrays, pointers, references and different qualifiers usually produces interesting test cases as well, that need to be handled properly.

Another thing that comes to mind with templates are overloaded operators.

template <class Foo>
void bar() {
     Foo x1, x2;
     Foo y = x1 + x2;
}

Builtins are not changed by operator+ but that can not be said about other types in general (maybe with concepts used properly).

mgehre marked 2 inline comments as done.May 12 2019, 11:21 PM
mgehre added inline comments.
clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp
209 ↗(On Diff #198882)
312 ↗(On Diff #198882)

The check only checks templates instantiations (so we will see no template parameters, just ordinary types). The plus here will be be function call in the AST of the instantiation when Foo has an overloaded operator+.
The current version will never propose to make bar() const when a method on this is called.
I can add the test to show this.

JonasToth added inline comments.May 13 2019, 11:29 AM
clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp
209 ↗(On Diff #198882)

Thank you C++ for showing my simple mindedness all the time :D
Its fine then and I will keep it in my mind for future test cases I check against ;)

312 ↗(On Diff #198882)

Yes please add a test to show they are not analyzed.

mgehre marked 17 inline comments as done.May 13 2019, 2:33 PM
mgehre added inline comments.
clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp
312 ↗(On Diff #198882)

I will now restrict this PR to the static part of the check, and I fail to come up with an example where operators could make a static looking function non-static.

mgehre updated this revision to Diff 199329.May 13 2019, 2:33 PM
mgehre marked an inline comment as done.

Remove `const` part from this PR. Address comments.

mgehre added a comment.Jul 2 2019, 1:08 PM

Friendly ping. Are there outstanding concerns that I could address?

aaron.ballman added inline comments.Jul 3 2019, 8:02 AM
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
96 ↗(On Diff #199329)

I'm not super keen on the check name. It's not very descriptive -- does this operate on static methods, does it make methods static, does it turn global dynamic initialization into static method initialization?

How about: readability-convert-functions-to-static or something more descriptive of the functionality?

clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp
25 ↗(On Diff #199329)

Do you need this constructor at all? If so, = default; it.

28–31 ↗(On Diff #199329)

Does this find a usage of this in unevaluated contexts? e.g.,

struct S {
  size_t derp() { return sizeof(this); }
};

I am hoping the answer is yes, because we wouldn't want to convert that into a static function. Probably worth a test.

56–57 ↗(On Diff #199329)

Under what circumstances can this happen?

58 ↗(On Diff #199329)

Can use auto here.

63 ↗(On Diff #199329)

SourceRange Range{...};

68 ↗(On Diff #199329)

Don't use auto here.

87–108 ↗(On Diff #199329)

Much of this logic should be hoisted into the AST matcher code rather than handled one-off here. Some of it can be done directly, others may require new local matchers to be defined.

116 ↗(On Diff #199329)

Formatting is wrong here -- run the patch through clang-format.

136–149 ↗(On Diff #199329)

const isn't the only thing to worry about though, no? You need to handle things like:

struct S {
  void f() volatile;
  void g() &;
  void h() &&;
};

Another one I am curious about are computed noexcept specifications and whether those are handled properly. e.g.,

struct S {
  int i;
  void foo(SomeClass C) noexcept(noexcept(C + i));
};
clang-tools-extra/test/clang-tidy/readability-static-method.cpp
99 ↗(On Diff #199329)

This is too subtle for my tastes. Another way to trigger the conversion is a type cast, which is far more understandable to anyone reading the code a few years from now.

mgehre updated this revision to Diff 208505.Jul 8 2019, 2:17 PM
mgehre marked 11 inline comments as done.
  • Address review comments
mgehre marked 2 inline comments as done.Jul 8 2019, 2:17 PM
mgehre added inline comments.
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
96 ↗(On Diff #199329)

I agree that we should find a better name. With readability-convert-functions-to-static, I miss the difference between adding static linkage to a free function versus making a member function static.
How about readability-convert-member-functions-to-static or readability-convert-method-to-static?

clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp
25 ↗(On Diff #199329)

Removed

28–31 ↗(On Diff #199329)

I added a test

56–57 ↗(On Diff #199329)

I will check

136–149 ↗(On Diff #199329)

I added tests for those cases and disabled fix-it generation for them to keep the code simple (for now).

mgehre marked 2 inline comments as done.Jul 8 2019, 2:18 PM
mgehre marked an inline comment as not done.Jul 9 2019, 12:47 PM
mgehre updated this revision to Diff 208785.Jul 9 2019, 12:49 PM

Move checks into AST matchers

mgehre marked an inline comment as done.Jul 9 2019, 12:49 PM
mgehre retitled this revision from [clang-tidy] initial version of readability-static-const-method to [clang-tidy] initial version of readability-const-method.
aaron.ballman added inline comments.Jul 10 2019, 6:08 AM
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
96 ↗(On Diff #199329)

I think readability-convert-member-functions-to-static would be a good name.

clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp
136–149 ↗(On Diff #199329)

There's an obscure edge case missing:

struct S {
  void f() __restrict;
};
35–45 ↗(On Diff #208785)

Is there a reason we can't use cxxConstructorDecl(), cxxDestructorDecl(), and cxxConversionDecl() instead?

130 ↗(On Diff #208785)

What does this do for code like:

#define constantly_annoying volatile

struct S {
  void func() constantly_annoying {}
};
lebedev.ri resigned from this revision.Jul 10 2019, 2:55 PM

Sorry, it does not appear that i'm being useful in this review.

clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp
89–101 ↗(On Diff #208785)

I'm pretty sure you can do

unless(anyOf(isExpansionInSystemHeader(), isVirtual(), ...))
clang-tools-extra/test/clang-tidy/readability-static-method.cpp
103 ↗(On Diff #208785)

Is there a test where a lambda (variant with and without capturing this, either explicitly, or implicitly)
is within a non-static class function?

mgehre retitled this revision from [clang-tidy] initial version of readability-const-method to [clang-tidy] initial version of readability-convert-member-functions-to-static.Jul 11 2019, 3:25 PM
mgehre marked 8 inline comments as done.Jul 11 2019, 3:43 PM
mgehre updated this revision to Diff 209360.Jul 11 2019, 3:45 PM

Implement comments

This should address all remaining comments.

aaron.ballman accepted this revision.Jul 16 2019, 2:30 AM

LGTM aside from a tiny nit.

clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
110 ↗(On Diff #209360)

const auto *

This revision is now accepted and ready to land.Jul 16 2019, 2:30 AM

Thank you for the review, Aaron!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 2:20 PM