Scan the fields of BINFO for an exact match of DECL. If found, return
DECL. Otherwise, return NULL_TREE. DECL is really the type "tree". */
Should say
Scan the fields of BINFO for an exact match of DECL. If found, return
the field. Otherwise, return NULL_TREE. DECL is really the type "tree". */
On Mon, 15 Feb 2021 at 12:45, Anthony Sharp <anthonyshar...@gmail.com> wrote:
>
> Hi all,
>
> This overloaded functions issue has been a pain in the ass but I have
> found a solution that seems to work. I could really do with some
> feedback on it though since I'm not sure if there's a better way to do
> it that I am missing.
>
> Here's the problem. When a using statement causes an access failure,
> we need to find out WHICH using statement caused the access failure
> (this information is not given to enforce_access; it merely gets the
> ORIGINAL decl [i.e. the one that the using statement inherited]). To
> make matters worse, a USING_DECL does not seem to store any
> information about where it "came from", e.g. there's no ORIGINAL_DECL
> (USING_DECL) macro or anything like that. DECL_INITIAL doesn't seem to
> help (and is probably totally not related to the issue).
>
> So we need to do a long-winded lookup.
>
> First, we iterate through the USING_DECLs in the class where access
> failed (in the context of a parent that has caused the access failure
> because it has private access). For normal field variables, we COULD
> simply do a name lookup on the USING_DECL and compare it to the
> original DECL. That would be easy, since there can only be one
> variable field with the same name.
>
> However, that doesn't work for overloaded functions. Name lookup would
> return multiple results, making comparison meaningless.
>
> What we need is therefore not a name lookup, but a decl lookup. It
> sounds stupid, because what that basically means is looking for an
> exact match of a decl, which is intuitively stupid, because why would
> you look for an exact match of something you already have? But
> actually we can take advantage of a quirk that USING_DECLs have, which
> is that, when stripped, cp_tree_equal (stripped_using_decl,
> original_decl) returns true, even through stripped_using_decl and
> original_decl exist on different lines and in different places. In
> other words, a USING_DECL is the same as the original DECL it
> inherits, even though they are in different places (Unless I am just
> being really dumb?).
>
> Anyways, to summarise ...
>
> 1) We iterate through USING_DECLs in the class that caused a private
> access failure.
> 2) For each USING_DECL, we find the DECL that USING_DECL inherited.
> 2.1) To do this, we iterate through all fields of all base classes
> (possibly slow? but it is just diagnostics code afterall,
> so idk if that's a big deal)
> 2.2) We compare each FIELD against the USING_DECL. if equal, then
> we return FIELD.
> 3) if the DECL that USING_DECL inherited is equal to the diagnostic
> decl we were given in enforce_access, we return USING_DECL as being
> the source of the problem.
>
> In a perfect world, I guess the USING_DECL would store somewhere what
> it inherited as it was parsed. But I'm not sure if that's practical to
> do and I'm not sure it would be worth the effort considering it would
> probably be used only for this niche scenario. Donno.
>
> I have not regression tested this, but it does seem to work on the
> test case at least (which I've also included). Also please ignore
> formatting issues ATM.
>
> If you think this is a reasonable way to do it then I will tidy up the
> code, test it and make a patch and send it over. If anyone has any
> better ideas (probably), then please let me know. I did try the name
> lookup as Jason suggested but, as I say, in the case of overloaded
> functions it returns multiple values, so it didn't help in determining
> what DECL a USING_DECL inherited.
>
> BTW, I will eventually put find_decl_using_decl_inherits and
> lookup_decl_exact_match in search.c.
>
> Just for proof, here's the output from the testcase (hopefully it
> formats this correctly):
>
> /home/anthony/Desktop/using9.C: In member function ‘void C::f()’:
> /home/anthony/Desktop/using9.C:34:11: error: ‘int A1::comma()’ is
> private within this context
> 34 | comma();
> | ^
> /home/anthony/Desktop/using9.C:22:24: note: declared private here
> 22 | using A2::comma, A1::comma; // { dg-message "declared" }
> | ^~~~~
> /home/anthony/Desktop/using9.C:35:14: error: ‘int A1::separate()’ is
> private within this context
> 35 | separate(); // { dg-error "private" }
> | ^
> /home/anthony/Desktop/using9.C:25:13: note: declared private here
> 25 | using A1::separate; // { dg-message "declared" }
> | ^~~~~~~~
> /home/anthony/Desktop/using9.C:36:5: error: ‘int A2::alone’ is private
> within this context
> 36 | alone = 5; // { dg-error "private" }
> | ^~~~~
> /home/anthony/Desktop/using9.C:27:13: note: declared private here
> 27 | using A2::alone;
> | ^~~~~
>
> Actual code attached.
>
> Anthony
>
>
> On Tue, 9 Feb 2021 at 20:07, Jason Merrill <ja...@redhat.com> wrote:
> >
> > On 2/9/21 6:18 AM, Anthony Sharp wrote:
> > > 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.
> >
> > The comment is pretty conusing, I agree. But what it's saying is that
> > both arguments must be types, and one of those types must be BINFO_TYPE
> > (some_binfo).
> >
> > Your first line doesn't work because you're comparing a type and a
> > binfo. The second one doesn't work because you're comparing the binfo
> > for a most-derived object of the type to the binfo for a base subobject
> > of the same type, and those are different, because binfos represent
> > nodes in inheritance graphs.
> >
> > > 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 <ja...@redhat.com
> > > <mailto:ja...@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
> > >
> >
