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 <anthonyshar...@gmail.com> > > * 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 <anthonyshar...@gmail.com> > > * 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