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

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2017-04-25 at 18:22 -0400, Nathan Sidwell wrote:
> On 04/25/2017 04:01 PM, David Malcolm wrote:
> > On Tue, 2017-04-25 at 12:11 -0400, Nathan Sidwell wrote:
> > > On 04/25/2017 11:58 AM, David Malcolm wrote:
> > > 
> > > >     { return FIELD; }
> 
> > I tried adding the kind of filtering you suggest, but the binfo
> > doesn't
> > seem to have info on const vs non-const qualification of the
> > accessor.

Sorry, I misspoke slightly here; what I meant to say is that I don't
have access to the const vs non-const qualification of the *access*
i.e. whether the argument at the call site itself is const vs non
-const.

> You need to look at the type of the function-decl.  I think looking
> at 
> the artifical this parm on TYPE_ARG_TYPES?

Thanks - yes; that gives information on the const vs non-const of the
"this" parameter, but doesn't say whether the argument was const vs non
-const.

For example, in:

class t1
{
public:
  int& get_color () { return m_color; }

private:
  int m_color;
};

...we can see that t1::get_color does indeed take a non-const t1 as its
"this" ptr

However, within:

int test_const_ptr (const t1 *ptr)
{
  return ptr->m_color;
}

...when suggesting an accessor:

(gdb) bt
#0  locate_field_accessor (basetype_path=<tree_binfo 0x7ffff1a35c00>, field_decl=<field_decl 0x7ffff1a20ed8 m_color>, 
    foo_type=<record_type 0x7ffff1a1fdc8 t1>) at ../../src/gcc/cp/search.c:2190
#1  0x0000000000641d31 in maybe_suggest_accessor (basetype_path=<tree_binfo 0x7ffff1a35c00>, 
    field_decl=<field_decl 0x7ffff1a20ed8 m_color>, foo_type=<record_type 0x7ffff1a1fdc8 t1>) at ../../src/gcc/cp/call.c:6419
#2  0x000000000064221e in enforce_access (basetype_path=<tree_binfo 0x7ffff1a35c00>, decl=<field_decl 0x7ffff1a20ed8 m_color>, 
    diag_decl=<field_decl 0x7ffff1a20ed8 m_color>, complain=3, foo_type=<record_type 0x7ffff1a1fdc8 t1>)
    at ../../src/gcc/cp/call.c:6469
#3  0x0000000000892d92 in perform_or_defer_access_check (binfo=<tree_binfo 0x7ffff1a35c00>, 
    decl=<field_decl 0x7ffff1a20ed8 m_color>, diag_decl=<field_decl 0x7ffff1a20ed8 m_color>, complain=3, 
    foo_type=<record_type 0x7ffff1a1fdc8 t1>) at ../../src/gcc/cp/semantics.c:332
#4  0x000000000088b22c in lookup_member (xbasetype=<tree 0x0>, name=<identifier_node 0x7ffff1a35a80 m_color>, protect=1, 
    want_type=false, complain=3) at ../../src/gcc/cp/search.c:1339
#5  0x0000000000834740 in finish_class_member_access_expr (object=..., name=<identifier_node 0x7ffff1a35a80 m_color>, 
    template_p=false, complain=3) at ../../src/gcc/cp/typeck.c:2837
#6  0x00000000007c9b0c in cp_parser_postfix_dot_deref_expression (parser=0x7ffff7ffbbd0, token_type=CPP_DEREF, 
    postfix_expression=..., for_offsetof=false, idk=0x7fffffffcbfc, location=218274) at ../../src/gcc/cp/parser.c:7493


we have an INDIRECT_REF at cp_parser_postfix_dot_deref_expression:

7067		  postfix_expression
7068		    = cp_parser_postfix_dot_deref_expression
(parser, token->type,
7069							     
 postfix_expression,
7070							     
 false, &idk, loc);


from which we can see the const-ness of the t1:

(gdb) call debug_tree (postfix_expression.m_value)
 <parm_decl 0x7ffff1a37080 ptr
    type <pointer_type 0x7ffff1a1fc78
        type <record_type 0x7ffff1a1fbd0 t1 readonly type_5 type_6 SI
        [...etc...]

but the call to lookup_member from within
finish_class_member_access_expr discards this information, giving just
"access_path": a BINFO that wraps the RECORD_TYPE for t1 directly.

2836		  /* Look up the member.  */
2837		  member = lookup_member (access_path, name, /*protect=*/1,
2838					  /*want_type=*/false, complain);

so that at the point where we'd emit the hint, we don't have that
information anymore.

A somewhat invasive solution would be for lookup_member to grow an extra:
  tree object
parameter, and to pass this information down through the access
-enforcement code, so that locate_field_accessor can look at the const
-ness of the lookup, and avoid suggesting const methods when the object
is const.  The code would probably need to support the new param being
NULL_TREE for cases where we're looking up a static member.  Or maybe
an enum of access style for const vs non-const vs static.
Maybe name the param "access_hint" to signify that it's merely there
for the purpose of hints for the user, and not to affect the parsing
itself?

Another solution would be to not bother offering non-const methods as
accessors.

Thoughts?

Dave