[PATCH] Sema: Allow dll attributes on inline functions

[Aaron Ballman]

On Wed, Apr 2, 2014 at 12:28 PM, Nico Rieck <nico.rieck at gmail.com> wrote:
> On 02.04.2014 14:45, Aaron Ballman wrote:
>> Is the NB comment still applicable? I don't see anything converting
>> such a declaration into a dllexport.
>
> MSVC still does this, but we don't implement this weird behavior. Did
> you assume we do from the comment? Then I'll clarify it.

I assumed it was part of the FIXME. Clarification would be good.

>
>>
>>> +    bool IsExplicit =
>>> +      FD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization ||
>>> +      FD->getTemplateSpecializationKind() ==TSK_ExplicitInstantiationDefinition;
>>
>> Formatting is a bit weird here with the space missing -- is this how
>> clang-format handles it?
>
> I've often seen this done in LLVM when normal formatting overshoots by
> one char. clang-format most likely puts the enum value in the next line.

It's not a blocker, just looked weird.

>
>> err_attribute_can_be_applied_only_to_symbol_declaration is no longer
>> used, and can be removed from DiagnosticSemaKinds.td.
>
> Good catch, I'll remove it.

Thanks!

>
>> Something appears to be amiss with this patch -- the follow example
>> works in MSVC 2013 and creates an exported function, but causes a
>> fatal error in clang.
>
> That's because this patch is Sema-only, and the dllimport function is
> still getting linkonce_odr linkage instead of available_externally.
> I can try to split out the relevant IRGen changes and tests and put them
> up for review sooner.

That'd make me feel slightly more comfortable, if you can work it that way.

The only other thing I'd like to see is a test case for an inline
function in a class (like my example from above). I didn't see
anything in our existing tests, but I may have simply overlooked it.

Other than my minor nits, the patch LGTM, but I'd prefer if it was
committed after the IRGen changes are in.

Thanks!

~Aaron