[cfe-dev] [PATCH] Friend class cannot access protected field of parent

[John McCall]

On Jun 8, 2010, at 1:07 PM, Olivier Goffart wrote:

> Le Tuesday 08 June 2010, John McCall a écrit :
>> On Jun 8, 2010, at 11:29 AM, Olivier Goffart wrote:
>>> Le Tuesday 08 June 2010, John McCall a écrit :
>>>> On Jun 8, 2010, at 1:56 AM, Olivier Goffart wrote:
>>>>> Le Tuesday 08 June 2010, John McCall a écrit :
>>>>>> On Jun 8, 2010, at 12:23 AM, Olivier Goffart wrote:
>>>>>>> So you think it is better not to try to make it work, even for the
>>>>>>> non- template case?
>>>>>> 
>>>>>> Yes.  Like I said, I don't think this is an intended feature;  I think
>>>>>> it's a drafting error.  [class.access.base] contains many repetitions
>>>>>> of the phrase "member or friend";  I think someone just wrote "member
>>>>>> or friend" here without really thinking.  I think when they were
>>>>>> drafting the example in [class.protected] they were only thinking
>>>>>> about the effect of the protected access restriction, not whether the
>>>>>> member itself was accessible from the naming class.  I'm willing to
>>>>>> consider evidence in the form of a DR that I'm wrong, but I don't
>>>>>> really want to implement the rule, and I'm not going to implement it
>>>>>> as long as it's undecidable.
>>>>> 
>>>>> Alright.
>>>>> Still, according to the examples in [class.protected], it does not seem
>>>>> to be a simple mistake.
>>>> 
>>>> Right, I acknowledge that.
> 
> By that you mean you would accept the patch?

No, sorry, not without some more support.  It's a hard question — at what point does
an apparent drafting mistake in the standard become obligatory? — but I am
comfortable with my current answer here.

I was offering some feedback since you said you'd done this as a way of getting into
clang development.

Incidentally, even if we were going to support this, we would balk at adding an extra
pointer of storage to every FunctionDecl (and to a lesser degree, to every
CXXRecordDecl).  A much more acceptable storage solution would be a side-table
stored on the ASTContext.

>>>> For what it's worth, your patch looks pretty good, except (1) I don't
>>>> see how you're dealing with multiple declarations of the target
>>>> function/record
>>> 
>>> I don't know what you mean here, can you elaborate?
>> 
>> There should be one linked list of "reverse friends" per function or
>> record, right? Except every separate declaration of a record or function
>> becomes a separate Decl node, and each Decl node has its own list root. 
>> Your code inserts a FriendDecl into the reverse-friends list of whatever
>> declaration is associated with the friend declaration. For friend classes,
>> this will be whatever decl was active in scope at the time (or the decl
>> introduced by the elaborated-type-specifier if none was around).  For
>> friend functions, it'll always be the "target declaration" of the friend
>> declaration itself. The access-control code works with the canonical
>> declaration, so your code works only as long as that's the one you push
>> the friend into the reverse-friends chain of.
> 
> I thought it is always the canonical one. 
> (line 479 and 586, I take the canonical form.)
> Now, I'm new to clang code, so I might have forgot something.

Ah, no, I missed that, sorry.

We typically use assertions to document requirements like this.

> This case does not work:
> 
> class A;
> class B : public A {  friend class C;  friend void foo();};
> class A { protected: typedef int foo; };
> class C { A::foo x; };
> 
> It seems I do not support the fact it retroactively become the canonical decl.
> I guess I can find the point where it becomes canonical, and move the linked 
> list to it. Do you have any pointer on where that happens?

During startDefinition.

> (I'd also appreciate hints or suggestions for friend of template class (the 
> FIXME in the test))

You'd need to make a reverse-friend list for ClassTemplateDecls (and
FunctionTemplateDecls) and check whether the context class is associated
with a template.

Mostly, though, even if we were going to support this as a compatibility hack,
I see no reason to extend it to friend templates in the absence of the evidence
that anyone needs it.

John.