On Fri, Feb 22, 2013 at 5:31 AM, Ismail Pazarbasi <span dir="ltr"><<a href="mailto:ismail.pazarbasi@gmail.com" target="_blank">ismail.pazarbasi@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Fri, Feb 22, 2013 at 12:42 AM, Ismail Pazarbasi<br>
<div><div class="h5"><<a href="mailto:ismail.pazarbasi@gmail.com">ismail.pazarbasi@gmail.com</a>> wrote:<br>
> On Fri, Feb 22, 2013 at 12:18 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>> On Thu, Feb 21, 2013 at 3:11 PM, Ismail Pazarbasi<br>
>> <<a href="mailto:ismail.pazarbasi@gmail.com">ismail.pazarbasi@gmail.com</a>> wrote:<br>
>>><br>
>>> On Thu, Feb 21, 2013 at 11:34 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>>> wrote:<br>
>>> > Hi!<br>
>>> ><br>
>>> > On Thu, Feb 21, 2013 at 1:53 PM, Ismail Pazarbasi<br>
>>> > <<a href="mailto:ismail.pazarbasi@gmail.com">ismail.pazarbasi@gmail.com</a>> wrote:<br>
>>> >><br>
>>> >> Hi,<br>
>>> >><br>
>>> >> when operand of a noexcept operator in a friend function definition<br>
>>> >> involves a member of a template class enclosing the friend definition,<br>
>>> >> Clang casts current context to either a CXXMethodDecl or a<br>
>>> >> CXXRecordDecl. Since friend function is neither of these, cast fails.<br>
>>> >> The same test in a non-template class works fine.<br>
>>> >><br>
>>> >> The first patch tries to retrieve the lexical parent of the function<br>
>>> >> definition (per class.friend/p6-7), which is expected to be a record<br>
>>> >> type. A test is also provided (also, pasting below for reference).<br>
>>> >><br>
>>> >> template<typename T><br>
>>> >> class noexcept_member_operand_at_friend_definition {<br>
>>> >> T v_;<br>
>>> >> friend int add_to_v(noexcept_member_operand_at_friend_definition &t)<br>
>>> >> noexcept(noexcept(v_ + 42))<br>
>>> >> {<br>
>>> >> return t.v_ + 42;<br>
>>> >> }<br>
>>> >> };<br>
>>> >><br>
>>> >> void this_expr()<br>
>>> >> {<br>
>>> >> noexcept_member_operand_at_friend_definition<int> t;<br>
>>> >> add_to_v(t);<br>
>>> >> }<br>
>>> ><br>
>>> ><br>
>>> > Thanks for looking into this. There is a more fundamental problem here,<br>
>>> > which this patch is masking rather than fixing. Per 5.1.1/3, 'this' is<br>
>>> > not<br>
>>> > permitted to appear in the declaration of a friend function, so per<br>
>>> > 9.3.1/3,<br>
>>> > we should not transform the mention of 'v_' into a class member access.<br>
>>> > Therefore, we should build a DeclRefExpr for the member, not a<br>
>>> > CXXMemberExpr, in this case. It looks like the bug is that<br>
>>> > Parser::ParseFunctionDeclarator's IsCXX11MemberFunction is not taking<br>
>>> > 'friend' into account when determining whether a function in a class is<br>
>>> > a<br>
>>> > member function.<br>
>>> ><br>
>>> > To demonstrate the difference, here's a testcase which we should reject,<br>
>>> > but<br>
>>> > we currently accept (and still accept with your patch):<br>
>>> ><br>
>>> > struct S {<br>
>>> > friend auto f() -> decltype(this);<br>
>>> > };<br>
>>> Thank you for reviewing these.<br>
>>><br>
>>> I first thought we should have rejected this and the case with a<br>
>>> non-template class as well. But both GCC and Clang accepted, and I<br>
>>> honestly started to look what am I missing. I found class.friend/p6-7,<br>
>>> which has nothing to do with 'this' pointer, but only lexical scope;<br>
>>> so I thought name lookup can find identifier in the class' scope, and<br>
>>> because 'this' is not used inside the function definition itself, it<br>
>>> might be legal to use 'this' inside noexcept operator (which is<br>
>>> clearly not permitted by 5.1.1/3).<br>
>><br>
>><br>
>> Your example is valid. While you can't use 'this' in a friend declaration,<br>
>> you *can* use 'v_', because noexcept is an unevaluated context; see<br>
>> 5.1.1/13.<br>
><br>
> OK, thank you very much, I think I understood it. I will check what<br>
> can I do about IsCXX11MemberFunction function first.<br>
><br>
>>><br>
>>> ><br>
>>> >> The second patch is a minor one; it merges two if statements that have<br>
>>> >> the same condition.<br>
>>> ><br>
>>> ><br>
>>> > I don't think this patch is correct. Note that the body of the first<br>
>>> > 'if'<br>
>>> > statement can cause the condition of the second to no longer be 'true'.<br>
>>> Yes, you are right!<br>
>>> _______________________________________________<br>
>>> cfe-commits mailing list<br>
>>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>><br>
>><br>
<br>
</div></div>Hi again,<br>
<br>
As you said, IsCXX11MemberFunction was not taking 'friend' specifier<br>
into account. I have added the check, and expression is not<br>
transformed to a class member access expression.</blockquote><div><br></div><div>Great! Two things:</div><div><br></div><div>1) It would be a bit clearer to only check isFriendSpecified in the MemberContext case.</div><div>
2) Please include some tests (the test from your original testcase plus the test from my reply would be fine).</div><div><br></div><div>Then I'll check this fix in for you. Thanks!</div></div>