[cfe-commits] pr12328 PATCH: Friend method declaration doesn't require method to be visible

[David Blaikie]

On Wed, Apr 25, 2012 at 7:52 AM, Stepan Dyatkovskiy <stpworld at narod.ru> wrote:
> Hi John. I'm afraid that it will problematically to fix it for templates
> right now. Consider this example:
>
> template <class T>
> class X {
>   void f();
> };
>
> template<class T>
> class Y {
>   friend void X<T>::f();
> };
>
> If I'm get right here friend is declared in depended context. I need
> learn more how to lookup previous declarations in depended contexts.
> Currently in clang doesn't checks anything for friend declarations like
> this.
> Even
>
> friend void X<T>::uh_man_i_didnt_see_u_b4();
>
> will compiled.

Sorry, I haven't been following this whole thread - but I believe
these specific examples (where X is a template as above, not the cases
below where X is a non-template) are currently correct. You can't fail
to compile the examples you gave because X<T> might have explicit
specializations for some particular T that does have "void
uh_man_i_didnt_see_u_b4()" for example. And so long as you only
instantiate Y<T> with Ts such that X<T>'s specializations have those
operations, I would expect it should compile.

But perhaps I've missed some detail here,
- David

> Also note, that it also compiled with GCC without any errors. GCC also
> doesn't detect errors in next example, while clang does (after fix):
>
> // NOT a template.
> class X {
>   void f();
> };
>
> template<class T>
> class Y {
>   friend void X<T>::f(); // GCC passes. clang will emit
>                          // error after my patch.
>   friend void X<T>::f_not_found(); // GCC passes. clang emits error.
>
> };
>
> In short. I'd glad to write a fix when both X and Y are templates, but I
> need more time and I propose to do it in separated patch.
>
> -Stepan.
>
> Stepan Dyatkovskiy wrote:
>> Hi John. Ok, I propose two versions then.
>>
>> 1. Add "bool SuppressDelayedDiagnostics" param to the CheckLookupAccess
>> method, that will "false" by default. IMHO looks like a workaround.
>> 2. Extend DelayedDiagnostics with enable() feature.
>>
>> #1 is attached as pr12328.patch
>> #2 is attached as pr12328-2.patch
>>
>> Also I attached new + fixed tests in pr12328-testcases.patch.
>>
>> -Stepan.
>>
>> John McCall wrote:
>>> On Apr 19, 2012, at 3:45 AM, Stepan Dyatkovskiy wrote:
>>>> ping.
>>>> Stepan Dyatkovskiy wrote:
>>>>> ping.
>>>>> Stepan Dyatkovskiy wrote:
>>>>>> ping.
>>>>>> Stepan Dyatkovskiy wrote:
>>>>>>> Hi all.
>>>>>>> Please find patch in attachment for review.
>>>>>>> I suppose that this check was not implemented yet, since I couldn't
>>>>>>> found it anywhere. The friend method availability check was
>>>>>>> inserted in
>>>>>>> Sema::ActOnFriendFunctionDecl.
>>>>>>> I also added "DeclContext *FromContext" to the AccessedEntity
>>>>>>> class. It
>>>>>>> allows to ask DelayedDiagnostic to use context that was "current"
>>>>>>> when
>>>>>>> this diagnostics was requested. It is usefull in our case, since we
>>>>>>> requested diagnostics when we're parsing class with "friend"
>>>>>>> declaration, but diagnostics is invoked for class with definition of
>>>>>>> friend member itself.
>>>
>>> Sorry for the delay.
>>>
>>> Please include test cases when you submit patches. Also, I'm afraid your
>>> patch is out-of-date.
>>>
>>> + if (Previous.getResultKind() == LookupResult::Found&&
>>> + Previous.getFoundDecl()->isCXXClassMember())
>>> + CheckLookupAccess(Previous);
>>>
>>> This is a bit silly, because we know exactly what we're checking
>>> access to,
>>> and we're getting nothing out of delaying the diagnostic. It would be
>>> better
>>> to provide a different CheckFriendAccess entrypoint that happens to
>>> suppress diagnostic delay.
>>>
>>> Also, you'll need to do this same access check in
>>> SemaTemplateInstantiateDecl
>>> so that it happens in template instantiations.
>>>
>>> John.
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits