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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst:10
+The check conservatively tries to preserve logical costness in favor of
+physical costness. The only operations on ``this`` that this check considers
+to preserve logical constness are
----------------
mgehre wrote:
> gribozavr wrote:
> > 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.
> 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?
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)."


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst:49
+    }
+  };
+
----------------
It is not enough to just show this example. Please explain why calling private member functions is not considered to preserve logical constness.


================
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() {
----------------
mgehre wrote:
> 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.
> 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...




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074