> > I think at least something like "Improve private access checking." would > be better. No need to be verbose in the ChangeLog. :)
That sounds like a good idea, I will change it. Yup, this one. Awesome. Yeah, that can be a pain. Best if your editor does it for you. If you > use vim, you can use gcc/contrib/vimrc and then vim will do most of the > formatting for you. Aah I didn't know that, thanks for the tip! Anthony On Fri, 5 Feb 2021 at 17:46, Marek Polacek <pola...@redhat.com> wrote: > On Fri, Feb 05, 2021 at 05:28:07PM +0000, Anthony Sharp wrote: > > 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. > > I think at least something like "Improve private access checking." would > be better. No need to be verbose in the ChangeLog. :) > > > > 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. > > Yeah, it's customary to start a test with > // PR c++/19377 > > > > 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. > > Oh, I see now. > > > > 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. > > Fair enough. > > > > 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. > > Yup, this one. > > > 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. > > Yeah, that can be a pain. Best if your editor does it for you. If you > use vim, you can use gcc/contrib/vimrc and then vim will do most of the > formatting for you. > > > > 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. > > Sounds good, thanks! > > > On Fri, 5 Feb 2021 at 16:06, Marek Polacek <pola...@redhat.com> 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 <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 > > > > > > > Marek > >