Re: [PATCH] c++: private inheritance access diagnostics fix [PR17314]

[Jason Merrill <jason@redhat.com>]

On 1/21/21 2:28 PM, Anthony Sharp wrote:
> Hi Jason,
> 
> I've finally completed my copyright assignment form. I've attached it
> to this email for reference.
> 
>> You don't need write access to the main repository to use these commands
>> on your local copy.  One nice thing about git compared to svn is that
>> you don't need to touch the server for anything but push and pull.
>>
>> Incidentally, how are you producing your patch?  Maybe try git
>> format-patch instead.
> 
> The method I am using at the moment is the one Ranjit Mathew talks
> about here: http://rmathew.com/articles/gcj/crpatch.html. Actually,
> having just re-read it, it says: 'NOTE: This is not the “proper” or
> “official” way of creating and submitting patches - that process has
> been explained in detail elsewhere. That process requires one to use
> Subversion (SVN). The process described here is meant for “one-off
> hackers” or people who cannot use SVN for some reason or the other.'
> ... oops!
> 
> It's my fault kind of - the official GCC webpage
> (https://gcc.gnu.org/gitwrite.html) explaining how to do it is called
> 'Read-write Git access' so I assumed it was only relevant for people
> who have access to the repo, but I see that is not the case.
> 
> I've tried the git way of doing it and I'm attaching a new patch file
> that (hopefully) is better this time. Basically what I did was what
> you suggested:
> 
> git pull
> contrib/gcc-git-customization.sh
> (make changes)
> git add *
> git gcc-commit-mklog
> git gcc-commit-mklog --amend
> git format-patch -1 master
> 
> I also re-built the source just to make sure I hadn't messed anything
> up. I re-ran the C++ regression tests using make check-c and make
> check-c++. Whilst I did not do a before/after comparison of the
> results, I checked the FAILs in gcc.sum and g++.sum and they all
> looked like they had nothing to do with my code. All the code is the
> same as before, so I'm thinking it should be fine (I just wanted to be
> safe). Also checked against check_GNU_style.sh.
> 
> Assuming that's all fine, as for the code itself, there might well be
> some tweaks that could make it better, and so if that is the case then
> please let me know.

The code looks good, I just have some minor tweaks.  Thanks!

> +++ b/gcc/cp/semantics.c
...
> +extern access_kind access_in_type (tree type, tree decl);
...
> +static tree
> +get_parent_with_private_access (tree decl, tree binfo)

Instead of making access_in_type non-static, let's defiine 
get_parent_with_private_access in search.c and declare it in cp-tree.h 
(with the declarations of nearby search.c functions).

> +  /* If we have not already figured out why DECL is innaccessible...  */
...
> +  /* Couldn't figure out why DECL is innaccesible, so just say it's
> +  innaccessible.  */

Only one 'n' in inaccessible.

There are various minor formatting issues:

(https://www.gnu.org/prep/standards/standards.html#Formatting)

> +  /* Couldn't figure out why DECL is innaccesible, so just say it's
> +  innaccessible.  */

Subsequent lines of a comment should be indented to line up with the 
first line.  This applies to all your multi-line comments.

> -    {
> -      if (issue_error)
> -	error ("%q#D is private within this context", diag_decl);
> -      inform (DECL_SOURCE_LOCATION (diag_decl),
> -	      "declared private here");
> -    }
> +  {
> +    if (issue_error)
> +      error ("%q#D is private within this context", diag_decl);
> +    inform (DECL_SOURCE_LOCATION (diag_location), "declared private here");
> +  }

Don't change the indentation of these blocks; in the GNU coding style 
the { } are indented two spaces from the if.

> +	tree parent_binfo = get_parent_with_private_access (decl,
> +	  basetype_path);
...
> +       complain_about_access (decl, diag_decl, diag_location, true,
> +         parent_access);

The new line of arguments should be indented to line up with the first one.

Jason