[PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp]

>
> The parens are to help the editor line up the last line properly.  If
> the subexpression fits on one line, the parens aren't needed.


Ahhhh okay.

No; "compile" means translate from C++ to assembly, "assemble" means
> that, plus call the assembler to produce an object file.  Though since
> compilation errors out, assembling never happens.


lol I was being dumb and thinking it was the other way around. I will
change it.

  You could bring back your lookup from the earlier patch and look
> through the result to see if the function we're complaining about is
> part of what the particular using-decl brings in.


I will give that a try.

I think you want
> SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
>                     BINFO_TYPE (parent_binfo))


Am I reading this wrong then? :

/* Compare a BINFO_TYPE with another type for equality.
For a binfo, this is functionally equivalent to using same_type_p, but
measurably faster.
 At least one of the arguments must be a BINFO_TYPE.  The other can be a
BINFO_TYPE
or a regular type.  If BINFO_TYPE(T) ever stops being the main variant of
the class the
binfo is for, this macro must change.  */
#define SAME_BINFO_TYPE_P(A, B) ((A) == (B))

That leads me to believe that arguments A and B can be: BINFO, BINFO ... or
BINFO, TYPE ... or TYPE, BINFO. If so I don't see why this doesn't work:

(SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
and
(SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
parent_binfo))

Unless "BINFO_TYPE" is actually just how you refer to a regular class type
and not a BINFO. Seems a bit confusing to me.

This line needs one more space.


Oh I see ... so second line's arguments need to line up with the first
argument, not the first (.

I will send over a new patch later/tomorrow.

Anthony

On Tue, 9 Feb 2021 at 04:55, Jason Merrill <jason@redhat.com> wrote:

> On 2/5/21 12:28 PM, Anthony Sharp wrote:
> > Hi Marek,
> >
> >>> +      if ((TREE_CODE (parent_field) == USING_DECL)
> >>
> >> This first term doesn't need to be wrapped in ().
> >
> > I normally wouldn't use the (), but I think that's how Jason likes it
> > so that's why I did it. I guess it makes it more readable.
>
> Ah, no, I don't see any need for the extra () there.  When I asked for
> added parens previously:
>
> >> +       if (parent_binfo != NULL_TREE
> >> +           && context_for_name_lookup (decl)
> >> +           != BINFO_TYPE (parent_binfo))
> >
> > Here you want parens around the second != expression and its != token
> > aligned with "context"
>
> The parens are to help the editor line up the last line properly.  If
> the subexpression fits on one line, the parens aren't needed.
>
> >> We usually use dg-do compile.
> >
> > True, but wouldn't that technically be slower? I would agree if it
> > were more than just error-handling code.
>
> No; "compile" means translate from C++ to assembly, "assemble" means
> that, plus call the assembler to produce an object file.  Though since
> compilation errors out, assembling never happens.
>
> > +      if ((TREE_CODE (parent_field) == USING_DECL)
> > +        && TREE_PRIVATE (parent_field)
> > +        && (DECL_NAME (decl) == DECL_NAME (parent_field)))
> > +       return parent_field;
>
> Following our discussion about an earlier revision, this will often
> produce the right using-declaration, but might not if two functions of
> the same name are imported from different bases.  If I split the
> using-declaration in using9.C into two, i.e.
>
> >   using A2::i; // { dg-message "declared" }
>
> >   using A::i;
>
> then I get
>
> > wa.ii: In member function ‘void C::f()’:
> > wa.ii:28:7: error: ‘int A::i()’ is private within this context
> >    28 |     i();
> >       |       ^
> > wa.ii:20:13: note: declared private here
> >    20 |   using A2::i;
>
> pointing out the wrong using-declaration because it happens to be first.
>   You could bring back your lookup from the earlier patch and look
> through the result to see if the function we're complaining about is
> part of what the particular using-decl brings in.
>
> > I tried both:
> > (SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
> > and
> > (SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
> parent_binfo))
>
> I think you want
>
> SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
>                     BINFO_TYPE (parent_binfo))
>
> i.e. just change same_type_p to SAME_BINFO_TYPE_P.
>
> >           tree parent_binfo = get_parent_with_private_access (decl,
> > -
>  basetype_path);
> > +
> basetype_path);
>
> This line was indented properly before, and is changed to be indented
> one space too far.
>
> > +             diag_location = get_class_access_diagnostic_decl
> (parent_binfo,
> > +
> diag_decl);
>
> This line needs one more space.
>
> >           complain_about_access (decl, diag_decl, diag_location, true,
> > -                                parent_access);
> > +                               access_failure_reason);
>
> This line, too.
>
> > +};
> > \ No newline at end of file
>
> Let's add a newline.
>
> Jason
>
>