On Wed, Apr 2, 2014 at 12:28 PM, Nico Rieck <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
