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

Reply via email to