Page MenuHomePhabricator

Allow 'static' storage specifier on an out-of-line member function template declaration in MSVCCompat mode
ClosedPublic

Authored by Manna on Jan 8 2019, 8:28 PM.

Details

Summary

Microsoft compiler permits the use of 'static' storage specifier outside of a class definition if it's on an out-of-line member function template declaration.

This patch allows 'static' storage specifier on an out-of-line member function template declaration with a warning in Clang (To be compatible with Microsoft).

Intel C/C++ compiler allows the 'static' keyword with a warning in Microsoft mode. GCC allows this with -fpermissive.

Diff Detail

Repository
rL LLVM

Event Timeline

Manna created this revision.Jan 8 2019, 8:28 PM
Manna edited the summary of this revision. (Show Details)Jan 8 2019, 8:33 PM
mibintc requested changes to this revision.Jan 9 2019, 12:15 PM

More info: gcc allows this with -fpermissive, Microsoft gives no message regardless of /permissive-

This revision now requires changes to proceed.Jan 9 2019, 12:15 PM
aaron.ballman added inline comments.Jan 10 2019, 12:19 PM
include/clang/Basic/DiagnosticSemaKinds.td
1593 ↗(On Diff #180786)

Should be an ExtWarn<> instead of Warning<> and should be named ext_static_out_of_line.

1594 ↗(On Diff #180786)

Nit: instead of duplicating the text, you can use err_static_out_of_line.Text instead.

lib/Sema/SemaDecl.cpp
8629 ↗(On Diff #180786)

of 'static' -> of a 'static'

8633 ↗(On Diff #180786)

I would use a ternary operator here instead of duplicating the code. e.g.,

Diag(D.getDeclSpec().getStorageClassSpecLoc(),
     NewFD->getDescribedFunctionTemplate() && getLangOpts().MSVCCompat ? diag::warn_static_out_of_line : diag::err_static_out_of_line)
  << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());

(only with better formatting)

test/SemaCXX/warn-static-outside-class-definition.cpp
1 ↗(On Diff #180786)

Remove -fms-extensions from the RUN line as this is a compatibility feature

3 ↗(On Diff #180786)

You should run this through clang-format to match the formatting guidelines.

rnk added a comment.Jan 14 2019, 5:51 PM

Functionality seems reasonable, I'll let @aaron.ballman finish the review.

Manna updated this revision to Diff 183230.Jan 23 2019, 7:11 PM
Manna edited the summary of this revision. (Show Details)

Thanks for the feedback. Patch is updated based on review comments.

Manna marked 5 inline comments as done.Jan 23 2019, 7:12 PM
Manna marked an inline comment as done.Jan 23 2019, 7:15 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 25 2019, 9:02 AM
This revision was automatically updated to reflect the committed changes.

I feel like pointing out that this review happened with no lists subscribed, so it is practically invalid.

rnk added a comment.Jan 25 2019, 10:13 AM

I feel like pointing out that this review happened with no lists subscribed, so it is practically invalid.

Fair enough, but there were two appropriate reviewers who approved it. I think it was an honest, fairly common mistake. Unless you have objections, I don't see a need to revert and have more discussion.