Warn on duplicate function specifier

[sepavloff]

Summary

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

Please review.
Thank you.

[rsmith]

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]

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]

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]

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 by commit rL194559 (authored by @sepavloff).

cfe/trunk/include/clang/Sema/DeclSpec.h

private:
   unsigned TypeAltiVecBool : 1;
   unsigned TypeSpecOwned : 1;
 
   // type-qualifiers
   unsigned TypeQualifiers : 4;  // Bitwise OR of TQ.
 
   // function-specifier
   unsigned FS_inline_specified : 1;
+  unsigned FS_forceinline_specified: 1;
   unsigned FS_virtual_specified : 1;
   unsigned FS_explicit_specified : 1;
   unsigned FS_noreturn_specified : 1;
 
   // friend-specifier
   unsigned Friend_specified : 1;
 
   // constexpr-specifier
private:
   /// TSTNameLoc - If TypeSpecType is any of class, enum, struct, union,
   /// typename, then this is the location of the named type (if present);
   /// otherwise, it is the same as TSTLoc. Hence, the pair TSTLoc and
   /// TSTNameLoc provides source range info for tag types.
   SourceLocation TSTNameLoc;
   SourceRange TypeofParensRange;
   SourceLocation TQ_constLoc, TQ_restrictLoc, TQ_volatileLoc, TQ_atomicLoc;
   SourceLocation FS_inlineLoc, FS_virtualLoc, FS_explicitLoc, FS_noreturnLoc;
+  SourceLocation FS_forceinlineLoc;
   SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc;
 
   WrittenBuiltinSpecs writtenBS;
   void SaveWrittenBuiltinSpecs();
 
   ObjCDeclSpec *ObjCQualifiers;
 
   static bool isTypeRep(TST T) {
: StorageClassSpec(SCS_unspecified),
       TypeSpecSign(TSS_unspecified),
       TypeSpecType(TST_unspecified),
       TypeAltiVecVector(false),
       TypeAltiVecPixel(false),
       TypeAltiVecBool(false),
       TypeSpecOwned(false),
       TypeQualifiers(TQ_unspecified),
       FS_inline_specified(false),
+      FS_forceinline_specified(false),
       FS_virtual_specified(false),
       FS_explicit_specified(false),
       FS_noreturn_specified(false),
       Friend_specified(false),
       Constexpr_specified(false),
       Attrs(attrFactory),
       ProtocolQualifiers(0),
       NumProtocolQualifiers(0),
void ClearTypeQualifiers() {
     TypeQualifiers = 0;
     TQ_constLoc = SourceLocation();
     TQ_restrictLoc = SourceLocation();
     TQ_volatileLoc = SourceLocation();
     TQ_atomicLoc = SourceLocation();
   }
 
   // function-specifier
-  bool isInlineSpecified() const { return FS_inline_specified; }
-  SourceLocation getInlineSpecLoc() const { return FS_inlineLoc; }
+  bool isInlineSpecified() const {
+    return FS_inline_specified | FS_forceinline_specified;
+  }
+  SourceLocation getInlineSpecLoc() const {
+    return FS_inline_specified ? FS_inlineLoc : FS_forceinlineLoc;
+  }
 
   bool isVirtualSpecified() const { return FS_virtual_specified; }
   SourceLocation getVirtualSpecLoc() const { return FS_virtualLoc; }
 
   bool isExplicitSpecified() const { return FS_explicit_specified; }
   SourceLocation getExplicitSpecLoc() const { return FS_explicitLoc; }
 
   bool isNoreturnSpecified() const { return FS_noreturn_specified; }
   SourceLocation getNoreturnSpecLoc() const { return FS_noreturnLoc; }
 
   void ClearFunctionSpecs() {
     FS_inline_specified = false;
     FS_inlineLoc = SourceLocation();
+    FS_forceinline_specified = false;
+    FS_forceinlineLoc = SourceLocation();
     FS_virtual_specified = false;
     FS_virtualLoc = SourceLocation();
     FS_explicit_specified = false;
     FS_explicitLoc = SourceLocation();
     FS_noreturn_specified = false;
     FS_noreturnLoc = SourceLocation();
   }
 
public:
   void UpdateExprRep(Expr *Rep) {
     assert(isExprRep((TST) TypeSpecType));
     ExprRep = Rep;
   }
 
   bool SetTypeQual(TQ T, SourceLocation Loc, const char *&PrevSpec,
                    unsigned &DiagID, const LangOptions &Lang);
 
-  bool setFunctionSpecInline(SourceLocation Loc);
-  bool setFunctionSpecVirtual(SourceLocation Loc);
-  bool setFunctionSpecExplicit(SourceLocation Loc);
-  bool setFunctionSpecNoreturn(SourceLocation Loc);
+  bool setFunctionSpecInline(SourceLocation Loc, const char *&PrevSpec,
+                             unsigned &DiagID);
+  bool setFunctionSpecForceInline(SourceLocation Loc, const char *&PrevSpec,
+                                  unsigned &DiagID);
+  bool setFunctionSpecVirtual(SourceLocation Loc, const char *&PrevSpec,
+                              unsigned &DiagID);
+  bool setFunctionSpecExplicit(SourceLocation Loc, const char *&PrevSpec,
+                               unsigned &DiagID);
+  bool setFunctionSpecNoreturn(SourceLocation Loc, const char *&PrevSpec,
+                               unsigned &DiagID);
 
   bool SetFriendSpec(SourceLocation Loc, const char *&PrevSpec,
                      unsigned &DiagID);
   bool setModulePrivateSpec(SourceLocation Loc, const char *&PrevSpec,
                             unsigned &DiagID);
   bool SetConstexprSpec(SourceLocation Loc, const char *&PrevSpec,
                         unsigned &DiagID);
 

cfe/trunk/lib/Parse/ParseDecl.cpp

while (1) {
 
     // Microsoft declspec support.
     case tok::kw___declspec:
       ParseMicrosoftDeclSpec(DS.getAttributes());
       continue;
 
     // Microsoft single token adornments.
     case tok::kw___forceinline: {
-      isInvalid = DS.setFunctionSpecInline(Loc);
+      isInvalid = DS.setFunctionSpecForceInline(Loc, PrevSpec, DiagID);
       IdentifierInfo *AttrName = Tok.getIdentifierInfo();
       SourceLocation AttrNameLoc = Tok.getLocation();
       // FIXME: This does not work correctly if it is set to be a declspec
       //        attribute, and a GNU attribute is simply incorrect.
       DS.getAttributes().addNew(AttrName, AttrNameLoc, 0, AttrNameLoc, 0, 0,
                                 AttributeList::AS_GNU);
       break;
     }
case tok::kw_thread_local:
       break;
     case tok::kw__Thread_local:
       isInvalid = DS.SetStorageClassSpecThread(DeclSpec::TSCS__Thread_local,
                                                Loc, PrevSpec, DiagID);
       break;
 
     // function-specifier
     case tok::kw_inline:
-      isInvalid = DS.setFunctionSpecInline(Loc);
+      isInvalid = DS.setFunctionSpecInline(Loc, PrevSpec, DiagID);
       break;
     case tok::kw_virtual:
-      isInvalid = DS.setFunctionSpecVirtual(Loc);
+      isInvalid = DS.setFunctionSpecVirtual(Loc, PrevSpec, DiagID);
       break;
     case tok::kw_explicit:
-      isInvalid = DS.setFunctionSpecExplicit(Loc);
+      isInvalid = DS.setFunctionSpecExplicit(Loc, PrevSpec, DiagID);
       break;
     case tok::kw__Noreturn:
       if (!getLangOpts().C11)
         Diag(Loc, diag::ext_c11_noreturn);
-      isInvalid = DS.setFunctionSpecNoreturn(Loc);
+      isInvalid = DS.setFunctionSpecNoreturn(Loc, PrevSpec, DiagID);
       break;
 
     // alignment-specifier
     case tok::kw__Alignas:
       if (!getLangOpts().C11)
         Diag(Tok, diag::ext_c11_alignment) << Tok.getName();
       ParseAlignmentSpecifier(DS.getAttributes());
       continue;

cfe/trunk/lib/Sema/DeclSpec.cpp

unsigned DeclSpec::getParsedSpecifiers() const {
 
   if (TypeQualifiers != TQ_unspecified)
     Res |= PQ_TypeQualifier;
 
   if (hasTypeSpecifier())
     Res |= PQ_TypeSpecifier;
 
   if (FS_inline_specified || FS_virtual_specified || FS_explicit_specified ||
-      FS_noreturn_specified)
+      FS_noreturn_specified || FS_forceinline_specified)
     Res |= PQ_FunctionSpecifier;
   return Res;
 }
 
 template <class T> static bool BadSpecifier(T TNew, T TPrev,
                                             const char *&PrevSpec,
                                             unsigned &DiagID,
                                             bool IsExtension = true) {
bool DeclSpec::SetTypeSpecError() {
   TypeSpecOwned = false;
   TSTLoc = SourceLocation();
   TSTNameLoc = SourceLocation();
   return false;
 }
 
 bool DeclSpec::SetTypeQual(TQ T, SourceLocation Loc, const char *&PrevSpec,
                            unsigned &DiagID, const LangOptions &Lang) {
-  // Duplicates are permitted in C99, but are not permitted in C++. However,
-  // since this is likely not what the user intended, we will always warn.  We
-  // do not need to set the qualifier's location since we already have it.
+  // Duplicates are permitted in C99 onwards, but are not permitted in C89 or
+  // C++.  However, since this is likely not what the user intended, we will
+  // always warn.  We do not need to set the qualifier's location since we
+  // already have it.
   if (TypeQualifiers & T) {
     bool IsExtension = true;
     if (Lang.C99)
       IsExtension = false;
     return BadSpecifier(T, T, PrevSpec, DiagID, IsExtension);
   }
   TypeQualifiers |= T;
 
   switch (T) {
   case TQ_unspecified: break;
   case TQ_const:    TQ_constLoc = Loc; return false;
   case TQ_restrict: TQ_restrictLoc = Loc; return false;
   case TQ_volatile: TQ_volatileLoc = Loc; return false;
   case TQ_atomic:   TQ_atomicLoc = Loc; return false;
   }
 
   llvm_unreachable("Unknown type qualifier!");
 }
 
-bool DeclSpec::setFunctionSpecInline(SourceLocation Loc) {
-  // 'inline inline' is ok.
+bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *&PrevSpec,
+                                     unsigned &DiagID) {
+  // 'inline inline' is ok.  However, since this is likely not what the user
+  // intended, we will always warn, similar to duplicates of type qualifiers.
+  if (FS_inline_specified) {
+    DiagID = diag::warn_duplicate_declspec;
+    PrevSpec = "inline";
+    return true;
+  }
   FS_inline_specified = true;
   FS_inlineLoc = Loc;
   return false;
 }
 
-bool DeclSpec::setFunctionSpecVirtual(SourceLocation Loc) {
-  // 'virtual virtual' is ok.
+bool DeclSpec::setFunctionSpecForceInline(SourceLocation Loc, const char *&PrevSpec,
+                                          unsigned &DiagID) {
+  if (FS_forceinline_specified) {
+    DiagID = diag::warn_duplicate_declspec;
+    PrevSpec = "__forceinline";
+    return true;
+  }
+  FS_forceinline_specified = true;
+  FS_forceinlineLoc = Loc;
+  return false;
+}
+
+bool DeclSpec::setFunctionSpecVirtual(SourceLocation Loc,
+                                      const char *&PrevSpec,
+                                      unsigned &DiagID) {
+  // 'virtual virtual' is ok, but warn as this is likely not what the user
+  // intended.
+  if (FS_virtual_specified) {
+    DiagID = diag::warn_duplicate_declspec;
+    PrevSpec = "virtual";
+    return true;
+  }
   FS_virtual_specified = true;
   FS_virtualLoc = Loc;
   return false;
 }
 
-bool DeclSpec::setFunctionSpecExplicit(SourceLocation Loc) {
-  // 'explicit explicit' is ok.
+bool DeclSpec::setFunctionSpecExplicit(SourceLocation Loc,
+                                       const char *&PrevSpec,
+                                       unsigned &DiagID) {
+  // 'explicit explicit' is ok, but warn as this is likely not what the user
+  // intended.
+  if (FS_explicit_specified) {
+    DiagID = diag::warn_duplicate_declspec;
+    PrevSpec = "explicit";
+    return true;
+  }
   FS_explicit_specified = true;
   FS_explicitLoc = Loc;
   return false;
 }
 
-bool DeclSpec::setFunctionSpecNoreturn(SourceLocation Loc) {
-  // '_Noreturn _Noreturn' is ok.
+bool DeclSpec::setFunctionSpecNoreturn(SourceLocation Loc,
+                                       const char *&PrevSpec,
+                                       unsigned &DiagID) {
+  // '_Noreturn _Noreturn' is ok, but warn as this is likely not what the user
+  // intended.
+  if (FS_noreturn_specified) {
+    DiagID = diag::warn_duplicate_declspec;
+    PrevSpec = "_Noreturn";
+    return true;
+  }
   FS_noreturn_specified = true;
   FS_noreturnLoc = Loc;
   return false;
 }
 
 bool DeclSpec::SetFriendSpec(SourceLocation Loc, const char *&PrevSpec,
                              unsigned &DiagID) {
   if (Friend_specified) {

cfe/trunk/test/Parser/MicrosoftExtensions.c

#if 0
   __asm {
     mov eax, Bit
     mov ecx, Base
     lock bts [ecx], eax
     setc al
   };
 #endif
 }
+
+// Both inline and __forceinline is OK.
+inline void __forceinline pr8264() {
+}
+__forceinline void inline pr8264_1() {
+}
+void inline __forceinline pr8264_2() {
+}
+void __forceinline inline pr8264_3() {
+}
+// But duplicate __forceinline causes warning.
+void __forceinline __forceinline pr8264_4() {  // expected-warning{{duplicate '__forceinline' declaration specifier}}
+}
+
 _inline int foo99() { return 99; }
 
 void test_ms_alignof_alias() {
   unsigned int s = _alignof(int);
   s = __builtin_alignof(int);
 }
 
 void *_alloca(int);

cfe/trunk/test/Sema/declspec.c

 
 void test2() {}
 
 
 // PR6423
 struct test3s {
 } // expected-error {{expected ';' after struct}}
 typedef int test3g;
+
+// PR8264
+const const int pr8264_1 = 0;  // expected-warning {{duplicate 'const' declaration specifier}}
+volatile volatile int pr8264_2;  // expected-warning {{duplicate 'volatile' declaration specifier}}
+char * restrict restrict pr8264_3;  // expected-warning {{duplicate 'restrict' declaration specifier}}
+
+extern extern int pr8264_4;  // expected-warning {{duplicate 'extern' declaration specifier}}
+void pr8264_5() {
+  register register int x;  // expected-warning {{duplicate 'register' declaration specifier}}
+}
+
+inline inline void pr8264_6() {}  // expected-warning {{duplicate 'inline' declaration specifier}}
+_Noreturn _Noreturn void pr8264_7();  // expected-warning {{duplicate '_Noreturn' declaration specifier}}

cfe/trunk/test/SemaCXX/explicit.cpp

void testFunctionPointer()
     };
     
     FP    fp;
     NotFP nfp;
     fp(1);
     nfp(1); // expected-error {{type 'NotFP' does not provide a call operator}}
   }
 }
+
+namespace pr8264 {
+  struct Test {
+  explicit explicit Test(int x);  // expected-warning{{duplicate 'explicit' declaration specifier}}
+  };
+}

cfe/trunk/test/SemaCXX/virtuals.cpp

namespace rdar9670557 {
   typedef int func(int);
   func *a();
   struct X {
     virtual func f = 0;
     virtual func (g) = 0;
     func *h = 0;
   };
 }
+
+namespace pr8264 {
+  struct Test {
+    virtual virtual void func();  // expected-warning {{duplicate 'virtual' declaration specifier}}
+  };
+}