Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2017-04-25 at 07:49 -0400, Nathan Sidwell wrote:
> On 04/25/2017 07:46 AM, Nathan Sidwell wrote:
> > On 04/24/2017 04:06 PM, David Malcolm wrote:
> > 
> > > test.cc:12:13: note: field ‘int foo::m_field’ can be accessed via
> > > ‘int 
> > > foo::get_field() const’
> > >     return f->m_field;
> > >               ^~~~~~~
> > >               get_field()
> > > 
> > > Assuming that an IDE can offer to apply fix-it hints, this should
> > > make it easier to handle refactorings where one makes a field
> > > private and adds a getter.
> > > 
> > > It also helps by letting the user know that a getter exists, and
> > > the name of the getter ("is it "field", "get_field", etc?").
> > 
> > Neat!
> > 
> > > 
> > > OK for trunk?
> > > 
> > > gcc/cp/ChangeLog:
> > >     * call.c (maybe_suggest_accessor): New function.
> > >     (enforce_access): Call maybe_suggest_accessor for
> > > inaccessible
> > >     decls.
> > >     * cp-tree.h (locate_field_accessor): New decl.
> > >     * search.c (matches_code_and_type_p): New function.
> > >     (field_access_p): New function.
> > >     (direct_accessor_p): New function.
> > >     (reference_accessor_p): New function.
> > >     (field_accessor_p): New function.
> > >     (dfs_locate_field_accessor_pre): New function.
> > >     (locate_field_accessor): New function.
> > 
> > ok.
> 
> Oh, what if the field is being accessed for modification or
> lvalueness? 
> Must the accessor return T, or can it return 'T cv &'?  I.e. does it 
> need to look for setters too?

There's an example of doing so in the patch in:
  g++.dg/other/accessor-fixits-2.C

(see direct_accessor_p vs reference_accessor_p in the patch for how
this is handled).

The patch doesn't take account the context of how the returned field is
used, e.g. whether as an rvalue vs in a modification/lvalue way; it
just looks for a method of the form

  { return FIELD; }

for the correct field, favoring returning T to returning T&.

Is there a way to get at the "style" of access from
call.c:enforce_access?  (to determine if T vs T& would be a better
suggestion)

Otherwise, is the patch OK as-is?

A case the patch doesn't provide a hint for yet is for static data; e.g. for:

   return foo::s_singleton;

(as in g++.dg/other/accessor-fixits-3.C in the patch); would that be OK
to leave as a follow-up?

Thanks
Dave