[Bug c++/80711] warn on non-const accessor member functions

[Bug c++/80711] warn on non-const accessor member functions

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dcb314 at hotmail dot com

--- Comment #4 from Martin Sebor <msebor at gcc dot gnu.org> ---
*** Bug 94997 has been marked as a duplicate of this bug. ***

[Bug c++/80711] warn on non-const accessor member functions

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #5 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Martin Sebor from comment #3)
> There is a warning like that in the middle-end: -Wsuggest-attribute=pure. 

Whether a function is pure is a slightly different thing
to whether it is a C++ const member function.

I did a test build of current gcc with the warning
flag and libgomp has Werror set, so that prevents a full build.

However, for the partial build I did, the warning was produced about 160 times.

Is it worth mentioning that pure functions are a GNU only extension
but const member functions are full ISO C++ ?

[Bug c++/80711] warn on non-const accessor member functions

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #6 from Martin Sebor <msebor at gcc dot gnu.org> ---
You're right, there is a substantial difference between attributes const/pure
and constness in the C/C++ sense.  A warning that detects missing const on
member functions (i.e., this request) is implementable in the C++ front end,
while one that also considers accesses to the state of the object would have to
be implemented in the middle end (like -Wsuggest-attribute).  Only there is it
possible to tell if a pointer dereference accesses *this.

As an aside, a risk with enabling -Wsuggest-attribute= is that GCC doesn't
detect accesses that are invalid in pure functions (like to globals), or any
other misuses in functions declared with any of the suggested attributes. 
Since what is and isn't safe isn't always clear to everyone, it's easy to get
it wrong.  Once the detection is implemented, hopefully for GCC 11 (pr18487),
using attributes pure and const will be a lot safer.  But that's independent of
improving const-correctness.

[Bug c++/80711] warn on non-const accessor member functions

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #2)
> Also useful would be to warn for members that don't access any state at all:
> 
> struct indirect_cmp {
>   bool operator()(const X* l, const X* r) { return *l < *r; }
> };
> 
> This comparison object should have a const-qualified member function to be
> usable with associative containers such as std::set (see PR 83102 for
> example).

If I extend this to modify global state it's not pure, but should still be
const-qualified:

struct indirect_cmp {
  static int counter;
  bool operator()(const X* l, const X* r) {
    ++counter;
    return *l < *r;
  }
};

int indirect_cmp::counter = 0;

So the pure attribute isn't the right property.

[Bug c++/80711] warn on non-const accessor member functions

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #8 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Jonathan Wakely from comment #7)
> struct indirect_cmp {
>   static int counter;
>   bool operator()(const X* l, const X* r) {
>     ++counter;
>     return *l < *r;
>   }
> };
> 
> int indirect_cmp::counter = 0;
> 
> So the pure attribute isn't the right property.

I tried your code on cppcheck and much to my surprise it found
the problem:

$ /home/dcb/cppcheck/trunk//cppcheck --enable=all --inconclusive may9a.cc
may9a.cc:4:8: style:inconclusive: Technically the member function
'indirect_cmp::operator()' can be const. [functionConst]
  bool operator()(const X* l, const X* r) {
       ^
$

My working assumption had been that cppcheck was looking for C++
member functions that consist of a return statement only but clearly
it is doing more than that. 

If there is any interest, I could probably figure out the pattern of C++ 
code it is looking for.

My opinion is that a first approximation at implementation in gcc would
merely look for C++ member functions that are return statements only.
More fancy things could be done later in a second version.

[Bug c++/80711] warn on non-const accessor member functions

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to David Binderman from comment #8)
> My opinion is that a first approximation at implementation in gcc would
> merely look for C++ member functions that are return statements only.
> More fancy things could be done later in a second version.

No, that won't work:

struct Iota {
  int i = 0;
  int operator()() { return i++; }
};

[Bug c++/80711] warn on non-const accessor member functions

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #10 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Jonathan Wakely from comment #9)
> (In reply to David Binderman from comment #8)
> > My opinion is that a first approximation at implementation in gcc would
> > merely look for C++ member functions that are return statements only.
> > More fancy things could be done later in a second version.
> 
> No, that won't work:

Of course. Some false positives are to be expected with a first
approximation.

Perhaps I've been slightly less than clear. 

A first approximation isn't the final solution. It should
be a step towards a final solution.

[Bug c++/80711] warn on non-const accessor member functions

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I would expect no false positives for a warning like this.

[Bug c++/80711] warn on non-const accessor member functions

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
A first approximation that is implemented in the wrong part of the compiler,
using the wrong logic, giving the wrong answers, is not a step in the right
direction because it would need to be completely redone. Might as well just
start the right version and skip that first approximation.

[Bug c++/80711] warn on non-const accessor member functions

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80711

--- Comment #13 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Jonathan Wakely from comment #12)
> Might as well just start the right version and skip that first approximation.

It sounds to me like you are somewhat keen to implement.
Feel free to go right ahead. 

I'd be glad to help with some testing of the new feature.
Do you plan a test driven development, perhaps ?