[clang] Warning for inline constexpr functions

[Izaron]

Summary

"inline constexpr" and "inline consteval" function declarations are redundant and shall be written rather as "constexpr" and "consteval".

[Izaron]

I will provide more context in this comment.

Why this patch?
Clang currently has a warning for duplicating specifiers (inline inline, extern extern, consteval consteval, etc.), but has no warnings for redundant weaker specifiers.
I suggest to add such a group of warnings, starting from inline constexpr/inline consteval functions.

The Standard says (dcl.constexpr) that constexpr/consteval functions are implicitly inline. But Clang doesn't tell programmers that.
The volume of affected code can be seen in GitHub Search: inline constexpr, inline consteval.

What about the other meaning of inline?
Some years ago, the inline keyword meant not only "there may be multiple definitions", but also "there will be inlinehint attribute added in LLVM IR".
Fortunately, this is no more since Clang 3.3 release.
constexpr and inline constexpr functions emit the same LLVM IR attributes. (consteval function aren't even emitted to LLVM IR, executing exclusively at compile-time)

[Izaron]

Fix tests

[rjmccall]

I don't remember if this is written up anywhere, but clang has a longstanding policy that style enforcement belongs in a linter rather than the compiler. One programmer's "redundant" is another programmer's "clear and explicit". I don't think this warning is consistent with that.

[Izaron]

clang/test/Headers/x86-intrinsics-headers-clean.cpp

4–5

Honestly I have a doubt for this "fix" (just ignoring the new diagnostic), but I couldn't change the included file.

[Izaron]

In D117435#3247137, @rjmccall wrote:

I don't remember if this is written up anywhere, but clang has a longstanding policy that style enforcement belongs in a linter rather than the compiler. One programmer's "redundant" is another programmer's "clear and explicit". I don't think this warning is consistent with that.

Thanks, didn't know this. Hope I'll make it to clang-tidy then.
(There was an abandoned review back in time when "inline" meant "inlinehint" for LLVM IR. https://reviews.llvm.org/D18914)

clang/include/clang/Basic/DiagnosticCommonKinds.td

def ext_duplicate_declspec : Extension<"%sub{duplicate_declspec}0">,
   InGroup<DuplicateDeclSpecifier>;
 def ext_warn_duplicate_declspec : ExtWarn<"%sub{duplicate_declspec}0">,
   InGroup<DuplicateDeclSpecifier>;
 def warn_duplicate_declspec : Warning<"%sub{duplicate_declspec}0">,
   InGroup<DuplicateDeclSpecifier>;
 
 def err_duplicate_declspec : Error<"%sub{duplicate_declspec}0">;
 
+def warn_redundant_weaker_declspec_constexpr : Warning<
+  "'%0' declaration specifier is redundant due to the presence "
+  "of stricter '%sub{select_constexpr_spec_kind}1' declaration specifier">,
+  InGroup<RedundantDeclSpecifier>;
+
 def err_friend_decl_spec : Error<"'%0' is invalid in friend declarations">;
 
 def err_invalid_member_in_interface : Error<
   "%select{data member |non-public member function |static member function |"
           "user-declared constructor|user-declared destructor|operator |"
           "nested class }0%1 is not permitted within an interface type">;
 
 def err_attribute_uuid_malformed_guid : Error<

clang/include/clang/Basic/DiagnosticGroups.td

def TautologicalCompare : DiagGroup<"tautological-compare",
                                     [TautologicalConstantCompare,
                                      TautologicalPointerCompare,
                                      TautologicalOverlapCompare,
                                      TautologicalBitwiseCompare,
                                      TautologicalUndefinedCompare,
                                      TautologicalObjCBoolCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
+def RedundantDeclSpecifier : DiagGroup<"redundant-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;
 def XorUsedAsPow : DiagGroup<"xor-used-as-pow">;
 
 def Unsequenced : DiagGroup<"unsequenced">;
 // GCC name for -Wunsequenced

clang/include/clang/Sema/Sema.h

diagnoseIgnoredQualifiers(unsigned DiagID, unsigned Quals,
                             SourceLocation ConstQualLoc = SourceLocation(),
                             SourceLocation VolatileQualLoc = SourceLocation(),
                             SourceLocation RestrictQualLoc = SourceLocation(),
                             SourceLocation AtomicQualLoc = SourceLocation(),
                             SourceLocation UnalignedQualLoc = SourceLocation());
 
   static bool adjustContextForLocalExternDecl(DeclContext *&DC);
   void DiagnoseFunctionSpecifiers(const DeclSpec &DS);
+  void DiagnoseRedundantSpecifiers(QualType T, const DeclSpec &DS);
   NamedDecl *getShadowedDeclaration(const TypedefNameDecl *D,
                                     const LookupResult &R);
   NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult &R);
   NamedDecl *getShadowedDeclaration(const BindingDecl *D,
                                     const LookupResult &R);
   void CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
                    const LookupResult &R);
   void CheckShadow(Scope *S, VarDecl *D);

clang/lib/Sema/SemaDecl.cpp

if (DS.hasExplicitSpecifier())
     Diag(DS.getExplicitSpecLoc(),
          diag::err_explicit_non_function);
 
   if (DS.isNoreturnSpecified())
     Diag(DS.getNoreturnSpecLoc(),
          diag::err_noreturn_non_function);
 }
 
+/// Diagnose specifiers on a declaration of an identifier that
+/// are redundant due to the presence of stricter specifiers
+void Sema::DiagnoseRedundantSpecifiers(QualType T, const DeclSpec &DS) {
+  if (T->isFunctionType()) {
+    // [dcl.constexpr]p1: A function declared with the constexpr or consteval
+    // specifier is implicitly an inline function
+    if (DS.hasConstexprSpecifier() && DS.isInlineSpecified()) {
+      Diag(DS.getInlineSpecLoc(),
+           diag::warn_redundant_weaker_declspec_constexpr)
+          << "inline" << static_cast<int>(DS.getConstexprSpecifier());
+    }
+  }
+}
+
 NamedDecl*
 Sema::ActOnTypedefDeclarator(Scope* S, Declarator& D, DeclContext* DC,
                              TypeSourceInfo *TInfo, LookupResult &Previous) {
   // Typedef declarators cannot be qualified (C++ [dcl.meaning]p1).
   if (D.getCXXScopeSpec().isSet()) {
     Diag(D.getIdentifierLoc(), diag::err_qualified_typedef_declarator)
       << D.getCXXScopeSpec().getRange();
     D.setInvalidType();
Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
                               MultiTemplateParamsArg TemplateParamListsRef,
                               bool &AddToScope) {
   QualType R = TInfo->getType();
 
   assert(R->isFunctionType());
   if (R.getCanonicalType()->castAs<FunctionType>()->getCmseNSCallAttr())
     Diag(D.getIdentifierLoc(), diag::err_function_decl_cmse_ns_call);
 
+  DiagnoseRedundantSpecifiers(R, D.getDeclSpec());
+
   SmallVector<TemplateParameterList *, 4> TemplateParamLists;
   for (TemplateParameterList *TPL : TemplateParamListsRef)
     TemplateParamLists.push_back(TPL);
   if (TemplateParameterList *Invented = D.getInventedTemplateParameterList()) {
     if (!TemplateParamLists.empty() &&
         Invented->getDepth() == TemplateParamLists.back()->getDepth())
       TemplateParamLists.back() = Invented;
     else

clang/test/AST/ast-dump-openmp-begin-declare-variant_10.c

-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -verify -ast-dump %s       | FileCheck %s --check-prefix=C
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -verify -ast-dump %s -x c++| FileCheck %s --check-prefix=CXX
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-redundant-decl-specifier -fopenmp -verify -ast-dump %s       | FileCheck %s --check-prefix=C
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-redundant-decl-specifier -fopenmp -verify -ast-dump %s -x c++| FileCheck %s --check-prefix=CXX
 // expected-no-diagnostics
 
 #ifdef __cplusplus
 #define CONST constexpr
 #else
 #define CONST __attribute__((const))
 #endif
 

clang/test/Analysis/builtin_bitcast.cpp

 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -verify %s \
 // RUN:   -analyzer-checker=core,debug.ExprInspection
 
 template <typename T> void clang_analyzer_dump(T);
 
-__attribute__((always_inline)) static inline constexpr unsigned int _castf32_u32(float __A) {
+__attribute__((always_inline)) static constexpr unsigned int _castf32_u32(float __A) {
   return __builtin_bit_cast(unsigned int, __A); // no-warning
 }
 
 void test(int i) {
   _castf32_u32(42);
 
   float f = 42;
 

clang/test/CXX/temp/temp.deduct.guide/p1.cpp

 // expected-error@-2 {{storage class specified for a member}}
 #else
 // expected-error@-4 {{deduction guide cannot be declared 'extern'}}
 #endif
 mutable A(int(&)[27]) -> A<int>; // expected-error-re {{{{'mutable' cannot be applied to|illegal storage class on}} function}}
 virtual A(int(&)[28]) -> A<int>; // expected-error {{'virtual' can only appear on non-static member functions}}
 const A(int(&)[31]) -> A<int>; // expected-error {{deduction guide cannot be declared 'const'}}
 
-const volatile static constexpr inline A(int(&)[29]) -> A<int>; // expected-error {{deduction guide cannot be declared 'static inline constexpr const volatile'}}
+const volatile static constexpr inline A(int(&)[29]) -> A<int>; // expected-error {{deduction guide cannot be declared 'static inline constexpr const volatile'}} \
+                                                                // expected-warning {{'inline' declaration specifier is redundant due to the presence of stricter 'constexpr' declaration specifier}}
 
 A(int(&)[30]) const -> A<int>; // expected-error {{deduction guide cannot have 'const' qualifier}}
 
 // No definition is allowed.
 A(int(&)[40]) -> A<int> {} // expected-error {{deduction guide cannot have a function definition}}
 A(int(&)[41]) -> A<int> = default; // expected-error {{deduction guide cannot have a function definition}} expected-error {{only special member functions may be defaulted}}
 A(int(&)[42]) -> A<int> = delete; // expected-error {{deduction guide cannot have a function definition}}
 A(int(&)[43]) -> A<int> try {} catch (...) {} // expected-error {{deduction guide cannot have a function definition}}

clang/test/Headers/x86-intrinsics-headers-clean.cpp

 // Make sure the intrinsic headers compile cleanly with no warnings or errors.
 
 // RUN: %clang_cc1 -ffreestanding -triple x86_64-unknown-unknown -Wsystem-headers \
-// RUN:   -Wcast-qual -fsyntax-only -flax-vector-conversions=none -x c++ -verify %s
+// RUN:   -Wno-redundant-decl-specifier -Wcast-qual -fsyntax-only \
+// RUN:   -flax-vector-conversions=none -x c++ -verify %s
 
 // expected-no-diagnostics
 
 #include <x86intrin.h>

clang/test/Parser/cxx0x-decl.cpp

namespace DuplicateSpecifier {
   consteval constexpr int f1(); // expected-error {{cannot combine with previous 'consteval'}}
   constexpr consteval int f2(); // expected-error {{cannot combine with previous 'constexpr'}}
   consteval consteval int f3(); // expected-error {{duplicate 'consteval' declaration specifier}}
 
   constinit consteval int wat = 0; // expected-error {{cannot combine with previous 'constinit'}}
   consteval constinit int huh(); // expected-error {{cannot combine with previous 'consteval'}}
 }
 
+namespace RedundantSpecifier {
+  consteval inline int f1(); // expected-warning {{'inline' declaration specifier is redundant due to the presence of stricter 'consteval' declaration specifier}}
+  inline constexpr int f2(); // expected-warning {{'inline' declaration specifier is redundant due to the presence of stricter 'constexpr' declaration specifier}}
+  inline constexpr inline int f3(); // expected-warning {{'inline' declaration specifier is redundant due to the presence of stricter 'constexpr' declaration specifier}} \
+                                    // expected-warning {{duplicate 'inline' declaration specifier}}
+  template<typename T> inline consteval int f4(T t); // expected-warning {{'inline' declaration specifier is redundant due to the presence of stricter 'consteval' declaration specifier}}
+  inline constexpr int i1 = 0;
+}
+
 namespace ColonColonDecltype {
   struct S { struct T {}; };
   ::decltype(S())::T invalid; // expected-error {{expected unqualified-id}}
 }
 
 namespace AliasDeclEndLocation {
   template<typename T> struct A {};
   // Ensure that we correctly determine the end of this declaration to be the

clang/test/SemaCUDA/function-overload.cu

 
 // Implicit HD functions compute with H function and D function.
 // In host compilation, foo(0.0, 2) should resolve to X::foo<double, int>.
 // In device compilation, foo(0.0, 2) should resolve to foo(double, int).
 // In either case there should be no ambiguity.
 namespace TestImplicitHDWithHAndD {
   namespace X {
     inline double foo(double, double) { return 0;}
-    inline constexpr float foo(float, float) { return 1;}
-    inline constexpr long double foo(long double, long double) { return 2;}
-    template<typename _Tp, typename _Up> inline constexpr double foo(_Tp, _Up) { return 3;}
+    constexpr float foo(float, float) { return 1;}
+    constexpr long double foo(long double, long double) { return 2;}
+    template<typename _Tp, typename _Up> constexpr double foo(_Tp, _Up) { return 3;}
   };
   using X::foo;
   inline __device__ double foo(double, double) { return 4;}
   inline __device__ float foo(float, int) { return 5;}
   inline __device__ float foo(int, int) { return 6;}
   inline __device__ double foo(double, int) { return 7;}
   inline __device__ float foo(float, float) { return 9;}
   template<typename _Tp, typename _Up> inline __device__ double foo(_Tp, _Up) { return 10;}
 
   int g() {
     return [](){
     return foo(0.0, 2);
     }();
   }
 }

clang/test/SemaCXX/builtin-is-constant-evaluated.cpp

 // RUN: %clang_cc1 -std=c++2a -verify %s -fcxx-exceptions -Wno-constant-evaluated -triple=x86_64-linux-gnu
 
 #define fold(x) (__builtin_constant_p(x) ? (x) : (x))
 
 using size_t = decltype(sizeof(int));
 
 namespace std {
-inline constexpr bool is_constant_evaluated() noexcept {
+constexpr bool is_constant_evaluated() noexcept {
   return __builtin_is_constant_evaluated();
 }
 } // namespace std
 
 extern int dummy; // expected-note 1+ {{declared here}}
 
 static_assert(__builtin_is_constant_evaluated());
 static_assert(noexcept(__builtin_is_constant_evaluated()));

clang/test/SemaCXX/cxx2a-destroying-delete.cpp

 // RUN: %clang_cc1 -std=c++2a -fexceptions -verify %s
 // RUN: %clang_cc1 -std=c++2a  -verify %s
 
 namespace std {
   using size_t = decltype(sizeof(0));
   enum class align_val_t : size_t;
 
   struct destroying_delete_t {
     struct __construct { explicit __construct() = default; };
     explicit destroying_delete_t(__construct) {}
   };
 
-  inline constexpr destroying_delete_t destroying_delete(destroying_delete_t::__construct());
+  constexpr destroying_delete_t destroying_delete(destroying_delete_t::__construct());
 }
 
 void operator delete(void*, std::destroying_delete_t); // ok, just a placement delete
 
 struct A;
 void operator delete(A*, std::destroying_delete_t); // expected-error {{first parameter of 'operator delete' must have type 'void *'}}
 
 struct A {

clang/test/SemaCXX/extended-usual-deallocation-functions.cpp

 using size_t = decltype(sizeof(0));
 enum class align_val_t : size_t;
 
 struct destroying_delete_t {
   struct __construct { explicit __construct() = default; };
   explicit destroying_delete_t(__construct) {}
 };
 
-inline constexpr destroying_delete_t destroying_delete(destroying_delete_t::__construct());
+constexpr destroying_delete_t destroying_delete(destroying_delete_t::__construct());
 }
 
 // FIXME: Should destroying delete really be on in all dialects by default?
 struct A {
   void operator delete(void*) = delete;
   void operator delete(A*, std::destroying_delete_t) = delete; // expected-note {{deleted}}
 };
 void ATest(A* a) { delete a; } // expected-error {{deleted}}

clang/test/SemaCXX/static-assert-cxx17.cpp

 
 template <typename T>
 struct S2 {
   template <typename U, typename V>
   static inline constexpr bool var = global_inline_var<U, V>;
 };
 
 template <typename U, typename V>
-inline constexpr bool constexpr_return_false() {
+constexpr bool constexpr_return_false() {
   return false;
 }
 
 template <typename U, typename V>
 void foo() {
   static_assert(S1<U, V>::value);
   // expected-error@-1{{static_assert failed due to requirement 'S1<int, float>::value'}}
 }