[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126818#3563376 <https://reviews.llvm.org/D126818#3563376>, @erichkeane wrote:

> In D126818#3562355 <https://reviews.llvm.org/D126818#3562355>, @tahonermann wrote:
>
>>> Note we might be confused, the parens there aren't completely clear as to what your intent is.
>>
>> Well, I know that I'm confused and not clear what my intent is :)
>>
>> I asked the question because there appears to be an asymmetry between (temp.friend)p9 sentence 1 <http://eel.is/c++draft/temp.friend#9.sentence-1> and (temp.friend)p9 sentence 2 <http://eel.is/c++draft/temp.friend#9.sentence-2>. Sentence 1 applies to all constrained non-template friend declarations regardless of any template argument dependence while sentence 2 applies to just a subset of constrained friend function templates; those that have some amount of template dependence. The difference impacts when mangling differences are required.
>>
>> I spent some time analyzing how gcc, clang, and MSVC handle these different cases. See https://godbolt.org/z/85E5eMh3x. Search for FIXME? to find cases where I think one or more of the compilers is misbehaving or where it is unclear to me whether or how [temp.friend]p9 applies. Some highlights:
>>
>> - Some compilers diagnose some ill-formed cases when parsing the class template, others don't until the class template is instantiated. Not surprising.
>> - All of the compilers reject non-template friend function definitions with non-dependent constraints due to duplicate definitions, presumably in violation of [temp.friend]p9; see the `ff2()` example.
>> - All of the compilers reject non-template friend function definitions with dependent constraints due to duplicate definitions, presumably in violation of [temp.friend]p9; see the `ff6()` example.
>> - Name mangling is currently insufficient to differentiate (otherwise) non-dependent friend function templates with dependent constraints; see the `fft5()` and `fft6()` examples.
>> - None of the compilers reject some cases of non-definitions that should be rejected by [temp.friend]p9; see the `fft5()` and `fft7()` examples.
>
> Yes, I understand that Clang doesn't currently properly implement [temp.friend]p9, and it is my belief too that none of the other compilers get it right at the moment either.  Clang approximates it by just directly comparing partially instantiated constraints, so it gets it MOSTLY right.
>
> I believe I have all of your test cases in my implementation of [temp.friend]p9 as a part of the deferred concepts patch here: https://reviews.llvm.org/D126907
>
> Aaron, my, and at least 1 other core expert's reading was:
>
> -A non template friend declaration with a requires-clause shall be a definition.
>
>   >> That is, ALL non-template friends, no matter what the requires-clause depends on.
>
> -A friend function template with a constraint that depends on the template parameter from an enclosing template shall be a definition.
>
>   >> That is, when the declaration is a friend function template, it must be a definition ONLY IF it depends on a template param from an enclosing template.
>
> THOSE two sentences aren't particularly relevant, except for setting up the two groups (that is, ALL constrained non-template friends and constrained friend function templates where the constraint depends on an enclosing template).
>
> The third sentence is the valuable one to both of these:
>
> -Such a constrained friend function or function template declaration does not declare the same function or function template as a declaration in any other scope.
>
>   >> That is, any friend in either of those two groups from sentence 1 and sentence 2 do not conflict with the same lexical declaration in a different instantiation.
>
> THIS patch attempts to implement the mangling required in that last sentence, however it doesn't differentiate the "ONLY if it depends on a template param from an enclosing template", which I intend to fix in a future revision, once the infrastructure from my deferred concepts patch (see link above) has made it into the repo.

SO I believe my 'future revision' part in the above is actually no longer necessary.  We prohibit that with a Sema 'redefinition error': https://godbolt.org/z/1r549vfK4

So the question here is whether we want 'baz' in that godbolt link to use the 'F' mangling or not.  @rjmccall?  The 'easiest' thing to do I think (based on this patch) is to ALWAYS use the 'F' mangling for a friend with a constraint.

WDYT?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126818/new/

https://reviews.llvm.org/D126818