Bug 80711

Summary: warn on non-const accessor member functions
Product: gcc Reporter: Martin Sebor <msebor>
Component: c++Assignee: Not yet assigned to anyone <unassigned>
Status: NEW ---    
Severity: enhancement CC: daniel.kruegler, dcb314, egallager, fw, webrown.cpp
Priority: P3 Keywords: diagnostic
Version: 7.0   
Target Milestone: ---   
See Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62029
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83102
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18487
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2017-11-22 00:00:00
Bug Depends on:    
Bug Blocks: 87403    

Description Martin Sebor 2017-05-11 14:51:18 UTC
C++ accessor (and similar) member functions that return a value computed from one or more private data members without modifying the owning object can and should be declared const.  Doing so not only improves the const-correctness of code that relies on objects of the type, it also aids its analyzability.  It's easy (and not uncommon) to forget to declare accessors const.  GCC could help detect the missing const by issuing a warning on such accessors, similarly to how it helps detect candidates for attribute const and pure.

This is an enhancement to request to add such a warning.  The test case below illustrates where it would be issued and shows the similar -Wsuggest-attribute warning that the new one could be modeled on.

$ cat t.C && gcc -O2 -S -Wall -Wextra -Wsuggest-attribute=pure t.C
class Int
{
public:
  int get () { return val; }   // suggested warning: function can be declared const

private:
  int val;
};

int get_int (Int &i)
{
  return i.get ();
}
t.C: In function ‘int get_int(Int&)’:
t.C:10:5: warning: function might be candidate for attribute ‘pure’ [-Wsuggest-attribute=pure]
 int get_int (Int &i)
     ^~~~~~~
Comment 1 Jonathan Wakely 2017-05-11 15:53:03 UTC
Good idea.
Comment 2 Jonathan Wakely 2017-11-22 13:18:02 UTC
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).
Comment 3 Martin Sebor 2017-11-22 16:46:26 UTC
There is a warning like that in the middle-end: -Wsuggest-attribute=pure.  Unfortunately, it's only good for functions that are actually emitted (i.e., not for C++ inline functions).

$ cat t.C && gcc -O2 -S -Wall -Wsuggest-attribute=pure t.C
typedef int X;

struct indirect_cmp {
  bool operator()(const X* l, const X* r);
};

bool indirect_cmp::operator()(const X* l, const X* r) { return *l < *r; }
t.C: In member function ‘bool indirect_cmp::operator()(const X*, const X*)’:
t.C:7:6: warning: function might be candidate for attribute ‘pure’ [-Wsuggest-attribute=pure]
 bool indirect_cmp::operator()(const X* l, const X* r) { return *l < *r; }
      ^~~~~~~~~~~~
Comment 4 Martin Sebor 2020-05-08 15:57:44 UTC
*** Bug 94997 has been marked as a duplicate of this bug. ***
Comment 5 David Binderman 2020-05-08 17:55:36 UTC
(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++ ?
Comment 6 Martin Sebor 2020-05-09 00:05:54 UTC
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.
Comment 7 Jonathan Wakely 2020-05-09 08:25:26 UTC
(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.
Comment 8 David Binderman 2020-05-09 08:36:32 UTC
(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.
Comment 9 Jonathan Wakely 2020-05-09 18:55:30 UTC
(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++; }
};
Comment 10 David Binderman 2020-05-09 20:27:26 UTC
(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.
Comment 11 Jonathan Wakely 2020-05-10 08:42:24 UTC
I would expect no false positives for a warning like this.
Comment 12 Jonathan Wakely 2020-05-10 08:52:06 UTC
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.
Comment 13 David Binderman 2020-05-10 09:45:40 UTC
(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 ?