This is an archive of the discontinued LLVM Phabricator instance.

Warn on duplicate function specifier
ClosedPublic

Authored by sepavloff on Oct 25 2013, 8:43 AM.

Details

Summary

This patch fixes PR8264. Duplicate qualifiers already are diagnozed,
now the same diagnostics is issued for duplicate function specifiers.

Please review.
Thank you.

Diff Detail

Event Timeline

rsmith added inline comments.Oct 25 2013, 1:07 PM
include/clang/Sema/DeclSpec.h
638–644 ↗(On Diff #5157)

" &", not "& ", please.

lib/Parse/ParseDecl.cpp
2770 ↗(On Diff #5157)

Do we really want a diagnostic for combining inline with __forceinline? That combination doesn't seem unreasonable to me.

rnk added inline comments.Oct 25 2013, 1:36 PM
lib/Parse/ParseDecl.cpp
2770 ↗(On Diff #5157)

MSVC does something unreasonable like this:

void __forceinline inline foo() {} // expected-warning {{C4141: 'inline' : used more than once}}
void inline __forceinline bar() {} // nothing

Clang requires users to apply inline in conjunction with always_inline, which leads users to write code like this to make all compilers happy:

#ifdef _MSC_VER
# define PORT_ALWAS_INLINE __forceline
#else
# define PORT_ALWAYS_INLINE __attribute__((always_inline))
#endif
void inline PORT_ALWAYS_INLINE foo() {}

We *definitely* shouldn't warn on that regardless of the ifdef taken.

sepavloff updated this revision to Unknown Object (????).Oct 28 2013, 7:59 AM

Warn on duplicate function specifier

It looks like things are more complicated here.
Thank you for the comment and for the nice example.
In the updated patch __forceinline is considered as a separate specifier,
but its presence is reported same as for inline.

rsmith accepted this revision.Nov 12 2013, 11:37 AM

LGTM with some minor changes.

include/clang/Sema/DeclSpec.h
538–539 ↗(On Diff #5195)

Please be consistent with the formatting elsewhere in Clang and LLVM: only put a function body on the same line as the function declaration if the whole function body fits on one line.

lib/Sema/DeclSpec.cpp
742 ↗(On Diff #5195)

"C99 onwards"

748 ↗(On Diff #5195)

This is unnecessary: C99 is always set when C11 is. Please revert this change.

test/Parser/MicrosoftExtensions.c
34–36 ↗(On Diff #5195)

Also test the other way around here (__forceinline inline)?

sepavloff closed this revision.Nov 12 2013, 11:02 PM

Closed by commit rL194559 (authored by @sepavloff).