[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked an inline comment as done.
mgehre added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp:311
+  // This member function must stay non-const, even though
+  // it only calls other private const member functions.
+  int &get() {
----------------
gribozavr wrote:
> Is it because the const version already exists? Then that should be the rule (not calling a private helper function).
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68074/new/

https://reviews.llvm.org/D68074