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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
