This is an archive of the discontinued LLVM Phabricator instance.

[MSVC] Explicit specializations can be declared in any namespace (fix for http://llvm.org/PR13738)
ClosedPublic

Authored by alexfrol on Mar 12 2015, 2:03 AM.

Details

Summary

MS compiler emits no errors in case of explicit specializations outside declaration enclosing namespaces, even when language extensions are disabled.
The patch is to suppress errors and emit extension warnings if explicit specializations are not declared in the corresponding namespaces.

This fixes PR13738.

Diff Detail

Repository
rL LLVM

Event Timeline

alexfrol updated this revision to Diff 21809.Mar 12 2015, 2:03 AM
alexfrol retitled this revision from to MS compatibility - explicit specializations can be declared in any namespace.
alexfrol updated this object.
alexfrol edited the test plan for this revision. (Show Details)
alexfrol added a reviewer: rsmith.
alexfrol set the repository for this revision to rL LLVM.
alexfrol added a subscriber: Unknown Object (MLST).
alexfrol retitled this revision from MS compatibility - explicit specializations can be declared in any namespace to [MSVC] Explicit specializations can be declared in any namespace (fix for http://llvm.org/PR13738).Mar 12 2015, 6:21 AM
alexfrol added a reviewer: rnk.

Test case? Also I think we're trying to avoid implementing every random
extension from msvc, preferring to update choose where the number of
instances of code relying on the extension is small (& not in a platform
header) - do you have a particular case where this shows up a lot (such
that it's a substaintial burden to update the code to be conforming)?

I might be wrong about this, so others may come in to correct me, that's
just my vague understanding

What would you recommend to do with http://llvm.org/PR13738 then?

rnk edited edge metadata.Mar 17 2015, 9:39 AM

This seems like a pretty reasonable compatibility hack, given that we already recover from the error gracefully. Richard?

llvm/tools/clang/lib/Sema/SemaTemplate.cpp
5889 ↗(On Diff #21809)

Hm, I guess we already recover from this error.

rsmith edited edge metadata.Mar 17 2015, 2:20 PM

Yes, given that we already support this for error recovery, this seems to be exceptionally low-cost. I'm fine with supporting this as an extension.

rsmith added inline comments.Mar 17 2015, 2:21 PM
llvm/tools/clang/lib/Sema/SemaTemplate.cpp
5840 ↗(On Diff #21809)

Since this is a non-heinous, conforming extension (and even something that's been suggested for standardization), should it go under -fms-extensions not -fms-compatibility?

alexfrol updated this revision to Diff 22246.Mar 19 2015, 3:22 AM
alexfrol edited edge metadata.

Update after review.

rnk accepted this revision.Mar 19 2015, 10:09 AM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 19 2015, 10:09 AM
This revision was automatically updated to reflect the committed changes.