Hi Marek,
> Pedantically, "Altered function." is not very good, it should say what
> in enforce_access changed.
I would normally 100% agree, but the changes are a bit too complicated
to be concisely (and helpfully) described there. I think the patch
description covers it well enough; not sure what I would say without
having to write a big paragraph there.
> Let's move the test into g++.dg/cpp1z and call it using9.C.
Okie dokie - it's a bit hard to know where stuff's supposed to go in
that folder. I'll put a comment in the testcase mentioning pr19377
just in case there's ever a regression.
> I don't understand the "generate a diagnostic decl location". Maybe just
> "generate a diagnostic?"
get_class_access_diagnostic_decl returns a decl which is used as the
location that the error-producing code points to as the source of the
problem. It could be better - I will change it to say "Examine certain
special cases to find the decl that is the source of the problem" to
make it a bit clearer.
> These two comments can be merged into one.
Technically yes ... but I like how it is since in a very subtle way it
creates a sense of separation between the first two paragraphs and the
third. The first two paras are sort of an introduction and the third
begins to describe the code.
> I think for comparing a binfo with a type we should use SAME_BINFO_TYPE_P.
Okay, I think that simplifies the code a bit aswell.
> 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.
> This could be part of the if above.
Oops - I forgot to change that when I modified the code.
> Just "had" instead of "did had"?
Good spot - that's a spelling mistake on my part. Should be "did have".
> Looks like this line is indented too much (even in the newer patch).
You're right (if you meant access_failure_reason = ak_private), I will change.
If you mean get_class_access_diagnostic_decl (parent_binfo, diag_decl)
then I donno, because Linux Text Editor says both are on column 64.
To be honest, I'm sure there is a way to do it, but I'm not really
sure how to consistently align code. Every text/code editor/browser
seems to give different column numbers (and display it differently)
since they seem to all treat whitespace differently.
> 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.
> Best to replace both with
> // { dg-do compile { target c++17 } }
Okie dokie. I did see both being used and I wasn't sure which to go for.
I'll probably send another patch over tomorrow.
Anthony
On Fri, 5 Feb 2021 at 16:06, Marek Polacek <[email protected]> wrote:
>
> Hi,
>
> a few comments:
>
> On Fri, Feb 05, 2021 at 03:39:25PM +0000, Anthony Sharp via Gcc-patches wrote:
> > 2021-02-05 Anthony Sharp <[email protected]>
> >
> > * semantics.c (get_class_access_diagnostic_decl): New function.
> > (enforce_access): Altered function.
>
> Pedantically, "Altered function." is not very good, it should say what
> in enforce_access changed.
>
> > gcc/testsuite/ChangeLog:
> >
> > 2021-02-05 Anthony Sharp <[email protected]>
> >
> > * g++.dg/pr19377.C: New test.
>
> Let's move the test into g++.dg/cpp1z and call it using9.C.
>
> > gcc/cp/semantics.c | 89 ++++++++++++++++++++++++++--------
> > gcc/testsuite/g++.dg/pr19377.C | 28 +++++++++++
> > 2 files changed, 98 insertions(+), 19 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/pr19377.C
> >
> > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> > index 73834467fca..ffb627f08ea 100644
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -256,6 +256,58 @@ pop_to_parent_deferring_access_checks (void)
> > }
> > }
> >
> > +/* Called from enforce_access. A class has attempted (but failed) to
> > access
> > + DECL. It is already established that a baseclass of that class,
> > + PARENT_BINFO, has private access to DECL. Examine certain special
> > cases to
> > + generate a diagnostic decl location. If no special cases are found,
> > simply
>
> I don't understand the "generate a diagnostic decl location". Maybe just
> "generate a diagnostic?"
>
> > + return DECL. */
> > +
> > +static tree
> > +get_class_access_diagnostic_decl (tree parent_binfo, tree decl)
> > +{
> > + /* When a class is denied access to a decl in a baseclass, most of the
> > + time it is because the decl itself was declared as private at the
> > point
> > + of declaration. So, by default, DECL is at fault.
> > +
> > + However, in C++, there are (at least) two situations in which a decl
> > + can be private even though it was not originally defined as such. */
> > +
> > + /* These two situations only apply if a baseclass had private access to
> > + DECL (this function is only called if that is the case). We must
> > however
> > + also ensure that the reason the parent had private access wasn't
> > simply
> > + because it was declared as private in the parent. */
>
> These two comments can be merged into one.
>
> > + if (context_for_name_lookup (decl) == BINFO_TYPE (parent_binfo))
> > + return decl;
>
> I think for comparing a binfo with a type we should use SAME_BINFO_TYPE_P.
>
> > + /* 1. If the "using" keyword is used to inherit DECL within a baseclass,
> > + this may cause DECL to be private, so we return the using statement as
> > + the source of the problem.
> > +
> > + Scan the fields of PARENT_BINFO and see if there are any using decls.
> > If
> > + there are, see if they inherit DECL. If they do, that's where DECL
> > was
> > + truly declared private. */
> > + for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
> > + parent_field;
> > + parent_field = DECL_CHAIN (parent_field))
> > + {
> > + if ((TREE_CODE (parent_field) == USING_DECL)
>
> This first term doesn't need to be wrapped in ().
>
> > + && TREE_PRIVATE (parent_field))
> > + {
> > + if (DECL_NAME (decl) == DECL_NAME (parent_field))
>
> This could be part of the if above. And then we can drop the braces (in the
> if and for both).
>
> > + return parent_field;
> > + }
> > + }
> > +
> > + /* 2. If decl was privately inherited by a baseclass of the current
> > class,
> > + then decl will be inaccessible, even though it may originally have
> > + been accessible to deriving classes. In that case, the fault lies
> > with
> > + the baseclass that used a private inheritance, so we return the
> > + baseclass type as the source of the problem.
> > +
> > + Since this is the last check, we just assume it's true. */
> > + return TYPE_NAME (BINFO_TYPE (parent_binfo));
> > +}
> > +
> > /* If the current scope isn't allowed to access DECL along
> > BASETYPE_PATH, give an error, or if we're parsing a function or class
> > template, defer the access check to be performed at instantiation time.
> > @@ -317,34 +369,33 @@ enforce_access (tree basetype_path, tree decl, tree
> > diag_decl,
> > diag_decl = strip_inheriting_ctors (diag_decl);
> > if (complain & tf_error)
> > {
> > - /* We will usually want to point to the same place as
> > - diag_decl but not always. */
> > + access_kind access_failure_reason = ak_none;
> > +
> > + /* By default, using the decl as the source of the problem will
> > + usually give correct results. */
> > tree diag_location = diag_decl;
> > - access_kind parent_access = ak_none;
> >
> > - /* See if any of BASETYPE_PATH's parents had private access
> > - to DECL. If they did, that will tell us why we don't. */
> > + /* However, if a parent of BASETYPE_PATH had private access to decl,
> > + then it actually might be the case that the source of the problem
> > + is not DECL. */
> > tree parent_binfo = get_parent_with_private_access (decl,
> > - basetype_path);
> > + basetype_path);
> >
> > - /* If a parent had private access, then the diagnostic
> > - location DECL should be that of the parent class, since it
> > - failed to give suitable access by using a private
> > - inheritance. But if DECL was actually defined in the parent,
> > - it wasn't privately inherited, and so we don't need to do
> > - this, and complain_about_access will figure out what to
> > - do. */
> > - if (parent_binfo != NULL_TREE
> > - && (context_for_name_lookup (decl)
> > - != BINFO_TYPE (parent_binfo)))
> > + /* So if a parent did had private access, then we need to do special
>
> Just "had" instead of "did had"?
>
> > + checks to obtain the best diagnostic location decl. */
> > + if (parent_binfo != NULL_TREE)
> > {
> > - diag_location = TYPE_NAME (BINFO_TYPE (parent_binfo));
> > - parent_access = ak_private;
> > + diag_location = get_class_access_diagnostic_decl (parent_binfo,
> > + diag_decl);
> > +
> > + /* We also at this point know that the reason access failed was
> > + because decl was private. */
> > + access_failure_reason = ak_private;
>
> Looks like this line is indented too much (even in the newer patch).
>
> >
> > /* Finally, generate an error message. */
> > complain_about_access (decl, diag_decl, diag_location, true,
> > - parent_access);
> > + access_failure_reason);
> > }
> > if (afi)
> > afi->record_access_failure (basetype_path, decl, diag_decl);
> > diff --git a/gcc/testsuite/g++.dg/pr19377.C b/gcc/testsuite/g++.dg/pr19377.C
> > new file mode 100644
> > index 00000000000..fb023a33f82
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/pr19377.C
> > @@ -0,0 +1,28 @@
> > +/* { dg-do assemble } */
>
> We usually use dg-do compile.
>
> > +// { dg-options "-std=c++17" }
>
> Best to replace both with
>
> // { dg-do compile { target c++17 } }
>
> > +class A
> > +{
> > + protected:
> > + int i();
> > +};
> > +
> > +class A2
> > +{
> > + protected:
> > + int i(int a);
> > +};
> > +
> > +class B:private A, private A2
> > +{
> > + // Comma separated list only officially supported in c++17 and later.
> > + using A::i, A2::i; // { dg-message "declared" }
> > +};
> > +
> > +class C:public B
> > +{
> > + void f()
> > + {
> > + i(); // { dg-error "private" }
> > + }
> > +};
> > \ No newline at end of file
> > --
> > 2.25.1
> >
>
> Marek
>
