[PATCH] PR17337: Retained language linkage

[Alp Toker]

On 22/10/2013 23:04, Richard Smith wrote:
> On Tue, Oct 22, 2013 at 2:47 PM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
>
>     On 22/10/2013 20:26, Richard Smith wrote:
>>     +def ext_retained_language_linkage : Extension<
>>     +  "friend function %0 retaining previous language linkage is an
>>     extension">,
>>     +  InGroup<DiagGroup<"retained-language-linkage">>;
>>
>>     I'd like this diagnostic to be clearer that
>>
>>       extern "C" void f();
>>       struct S {
>>         friend void f();
>>       };
>>
>>     is fine but
>>
>>       extern "C" void f();
>>       extern "C++" {
>>         struct S {
>>           friend void f();
>>         };
>>       }
>>
>>     is the extension. Maybe add a note pointing at the innermost
>>     surrounding linkage specification that we're ignoring?
>
>     I don't think that's the right way to look at this change.
>
>     My interpretation of the spec is very much that friend function
>     declarations don't have a linkage spec in the first place because
>     they don't introduce a function name:
>
>     [dcl.link]p4:
>       In a linkage-specification,
>       the specified language linkage applies to the function  types  of  all
>       function declarators, function names, and variable names **introduced** by
>       the declaration(s).
>
>
> This wording was vague and has been fixed; you need a more recent
> draft of the standard =) Here's what the latest draft says.
>
> "In a linkage-specification, the specified language linkage applies to
> the function types of all function declarators, function names with
> external linkage, and variable names with external linkage *declared
> within* the linkage-specification."
>
> Even if the linkage-specification didn't apply to the function name,
> it would apply to the function type and again make the code ill-formed
> (but we don't implement that rule...)
>
>     On the other hand, highlighting the selected outer linkage spec
>     _would_ be nice to do when diagnosing linkage specs in general.
>
>     I have some patches back from when the linkage spec work started
>     in 2010 to improve diags. Thinking to get back to it, but either
>     way, digging up and passing around the LinkageSpecDecl belongs in
>     a separate patch.
>
>
> I don't think the current diagnostic is very clear. Friend functions
> retaining their previous language linkage is standard; overriding an
> explicit linkage specification with that of an earlier declaration is
> not. The diagnostic suggests that the rule is something other than
> what it actually is. For obscure rules like these, people use our
> diagnostics to learn how the language works, and we do them a
> disservice if we mislead them.

Alright Richard, you've made a compelling argument :-)

I'll give you that diagnostic but it'll still come in an additional
commit. I'd like to extract the linkage spec SourceLocations separately
in a way that's generally useful.

>
>>     +      // The friend object kind isn't yet complete so check IDNS
>>     directly.
>>     +      if (New->getIdentifierNamespace() &
>>     Decl::IDNS_OrdinaryFriend) {
>>
>>     We don't care whether it's FOK_Declared or FOK_Undeclared, so why
>>     not:
>>
>>       if (New->getFriendObjectKind() != FOK_None) {
>>
>>     ?
>
>     Sure, let's do that.
>
>     So, I'm looking to land this patch with the getFriendObjectKind()
>     change and then contining with general improvement of linkage spec
>     diagnostics.
>

Just as we've decided where to go with the diagnostic, I've now had
second thoughts about your suggested IDNS change..

Looking back, I chose

|if (New->getIdentifierNamespace() & Decl::IDNS_OrdinaryFriend)|

intentionally instead of

|if (New->getFriendObjectKind() != FOK_None)|

This was to make it absolutely clear that we only retain the linkage of
_function declarations_ and not of any other friend kind. If the
getFriendObjectKind() version gets folded back into
haveIncompatibleLanguageLinkages() and applied to VarDecls or
potentially other Decl kinds (as it was in the original patch) there's a
risk it'll introduce a bug.

So while we're splitting hairs, what do you think, stick with IDNS or go
to getFriendObjectKind()?


>
>     Let me know if you feel strongly enough about the diagnostic to
>     object to landing this in the next day or so.
>
>
> If you want to land the diagnostic improvement separately, that's fine
> by me.

Thanks for reviewing, it is appreciated.

Alp.


>  
>
>     On Tue, Oct 22, 2013 at 10:46 AM, Alp Toker <alp at nuanti.com
>     <mailto:alp at nuanti.com>>wrote:
>>
>>         Hello Richard,
>>
>>         With this patch, friend function declarations will retain the
>>         language
>>         linkage specified for previous declarations instead of
>>         emitting an error
>>         diagnostic.
>>
>>         The feature is known to be compatible with GCC and MSVC and
>>         permits a
>>         language to be specified indirectly where it cannot otherwise
>>         be written
>>         directly in class scope.
>>
>>         Further to the previous patch, this feature is now a clang
>>         extension so
>>         warnings can be enabled/disabled with a
>>         -Wretained-language-linkage flag
>>         while we seek clarifications to the language standard. Tests
>>         have been
>>         moved to SemaCXX/linkage-spec.cpp.
>>
>>         Alp.
>>
>>         --
>>         http://www.nuanti.com
>>         the browser experts
>>
>>
>
>     -- 
>     http://www.nuanti.com
>     the browser experts
>
>

-- 
http://www.nuanti.com
the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131022/92f8cbca/attachment.html>