Page MenuHomePhabricator

[clang-tidy] Add readability-make-member-function-const
ClosedPublic

Authored by mgehre on Sep 26 2019, 12:53 AM.

Details

Summary

Finds non-static member functions that can be made `const`
because the functions don't use `this` in a non-const way.

The check conservatively tries to preserve logical costness in favor of
physical costness. See readability-make-member-function-const.rst for more
details.

Diff Detail

Event Timeline

mgehre created this revision.Sep 26 2019, 12:53 AM

Nice! I went with a more brute force approach, and I also struggled with logical vs physical const-correctness

https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/store/constmethod.cxx

Awesome!
I believe i have asked this question in the convert-to-static diff - can ExprMutAnalyzer be used here?

See also PR21981 and D45444.

clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
94

Unnecessary empty line.

126

Unnecessary empty line.

214

Shouldn't check work only in C++?

clang-tools-extra/docs/ReleaseNotes.rst
144

Please synchronize with first sentence in documentation.

Eugene.Zelenko added a project: Restricted Project.Sep 26 2019, 7:01 AM
Eugene.Zelenko added subscribers: JonasToth, shuaiwang.
Eugene.Zelenko added inline comments.Sep 26 2019, 9:50 AM
clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
61

Return type is not obvious, so auto should not be used.

146

Is Cast changed, if not, const auto * should be used. Same in other places.

mgehre updated this revision to Diff 222331.Sep 29 2019, 10:35 AM
mgehre marked 6 inline comments as done.
  • Implement review comments
mgehre marked 6 inline comments as done.Sep 29 2019, 10:35 AM

Awesome!
I believe i have asked this question in the convert-to-static diff - can ExprMutAnalyzer be used here?

I believe in this case, we can get away with a simpler analysis. Because this is an prvalue,
there are less operations that can be done to it, and it seems easy to just list those here. Also, we want
to conservatively disallow "const" access to non-public member variables/functions, and I don't see
how this would elegantly integrate with using the ExprMutationAnalyzer.

I'm holding off on reviewing the code until we figure out what the rules are.

clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst
11

Sorry, it is unclear to me what it means: "the check [...] tries to do X in favor of Y"

Also unclear what logical/physical constness mean.

18

These rules need a justification; if not for the users, but for future maintainers.

For example, why isn't reading a private member variable OK? Why isn't calling a private member function OK?

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
312

Is it because the const version already exists? Then that should be the rule (not calling a private helper function).

mgehre marked an inline comment as done.Oct 9 2019, 12:02 PM
mgehre added inline comments.
clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
312

Let me try to explain. If it make senses what I say, I'll then update the documentation.
The terms logical const and physical const are from here: https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const

The const version is just here to illustrate how this pattern works. The important fact is that a private member function is not part of the public interface of the class.
I.e. users of the class were not able to call data() with a const object before (even though data() is declared as const) because data() is at the same time private.
We thus cannot assume that the constness of data() reflects the interface contract the the author of the class had in mind.

Here, e.g. the author clearly wanted to only give access to a non-const int& to users that possses a non-const version of *this.

The rule "don't make the method const if it already has a const overload" seems appealing, but its neither sufficient (we still need the rule we currently have about private data)
nor is is necessary once we have all other rules. I have successfully run this check on the LLVM code base, and there were not issues with clashing overloads after fix-its. (It found multiple hundred of valid fixits)

The second example how to see the issue with private data members is the Pimpl idiom:

class S {
  struct Impl {
   int d;
  };
  Impl *I;
public:
  void set(int v) {
    I->d = v;
  }
};

A human would not make the set() function const because we consider the value of Impl->d to be part of the value of the instance. Technically, we can make set() const because
it does not modify I. But that is not the author's intention. I try to have no false-positives in this check,
so I conservatively assume that an access to a private member that is not of builtin type does not preserve logical constness.
Note that the same happens when the type of I is changed to unique_ptr<Impl>. unique_ptr::operator-> is a const member function but returns a reference to non-const Impl,
and so does not preserve logical constness

We might be able to extend the check by tracking pointer use, but that's pretty difficult.
The only extension I did is for builtin types, i.e.int. When we read an int (in the AST that's an LvalueToRvalue conversion), then there is no way that this can eventually lead to a modification
of the state of the int, so it preserves logical constness.

Const use of public members is also ok because the user of the class already has access to them. We are not widening the contract by making a member function const that
(only) contains const use of public members.

mgehre marked 2 inline comments as done.Oct 9 2019, 12:07 PM
mgehre added inline comments.
clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst
11

I guess it should read tries to preserve logical constness instead of physical constness.

logical/physical constness is from here: https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-state
Are there more common terms for this or should I link or copy the explanation?

18

Sure! Let's first see if you agree to the rules based on the explanation in my other comment.

gribozavr added inline comments.Oct 11 2019, 2:43 AM
clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst
11

I think you should add a link, and change "in favor of" to "not" -- "This check tries to annotate methods according to logical constness (not physical constness)."

50

It is not enough to just show this example. Please explain why calling private member functions is not considered to preserve logical constness.

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
312

I.e. users of the class were not able to call data() with a const object before (even though data() is declared as const) because data() is at the same time private.

Agreed.

We thus cannot assume that the constness of data() reflects the interface contract the the author of the class had in mind.

I don't see how this follows. Many other, public, aspects of the class might also not reflect the interface that the author had in mind -- because people write bugs, because C++ is complex and people don't understand their API surface.

Here, e.g. the author clearly wanted to only give access to a non-const int& to users that possses a non-const version of *this.

How does this follow? int &data() const; means give an int& to anyone.

I conservatively assume that an access to a private member that is not of builtin type does not preserve logical constness.

This part makes sense. I don't think we even need to bring up pimpl here, just anything that has an indirection in its implementation is sufficient to illustrate the problem.

I try to have no false-positives in this check

OK, I get it -- the private member thing is not really a rule, it is a heuristic to decrease the number of false positives. Makes sense, I guess? However, as a user, I'd still be surprised. I don't know if the number of false negatives due to this heuristic is larger than the number of false positives.

Assuming you want to keep the rule, in the documentation I'd suggest to explain it like this:

This check will suggest to add a const qualifier to a non-const method only if this method does something that is already possible though the public interface on a const pointer to the object:
...list of specifics...

This check will also suggest to add a const qualifier to a non-const method if this method uses private data and functions in a limited number of ways where logical constness and physical constness coincide:
...list of specifics...

mgehre updated this revision to Diff 225804.Oct 20 2019, 2:31 PM
mgehre marked 6 inline comments as done.
  • Update documentation

Friendly Ping

gribozavr2 accepted this revision.Nov 1 2019, 10:59 AM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
181

I think you meant ((const S*)this)->f().

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
22

Did you mean Str &Sref?

169

Move up, close to other data members?

180

Move up, close to other data members?

248

"DependentBase"

256

"ClassTemplate"

263

"MemberFunctionTemplate"

This revision is now accepted and ready to land.Nov 1 2019, 10:59 AM
gribozavr2 added inline comments.Nov 1 2019, 8:48 PM
clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
322

Could you add tests for calls to members of this through member function pointers? I don't care about possible false negatives in that case, but the code shouldn't crash or produce false positives.

mgehre marked 8 inline comments as done.Nov 2 2019, 2:52 PM

Thanks, fixed!

This revision was automatically updated to reflect the committed changes.