Bug 49633 - Added warning for static inline global and namespaced declarations

[serberoth]

Summary

Bug 49633 - Added a warning for global and namespace declared variables that are declared as both static and inline.

[erichkeane]

clang/lib/Sema/SemaDecl.cpp

7127

First, what about this is C++17 specific? The inline and static relationship is older than C++17, right?

Also, are you sure that the warning/stuff in the 'else' isn't still valuable?

[serberoth]

clang/lib/Sema/SemaDecl.cpp

7127

Perhaps I misunderstood something in the bug write-up, the component for the ticket is C++17. Also there is the warning that inline variables are a C++17 extension that appears when you use the inline keyword on a variable (regardless of the appearance of the static keyword). I suppose that does not necessarily mean we cannot simply show both warnings, and maybe that is the right thing to do. I felt that was not really necessary because of the other warning.

As for the other warnings in the else the one is the warning that I mentioned (which only applies when the C++17 standard is not applied, and the other is a C++14 compatibility warning which states:
"inline variables are incompatible with C++ standards before C++17"

You can see the messages for those diagnostic message here:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L1467

Please let me know if I am still missing something with how this diagnostic was supposed to apply and where. Thank you.

[rsmith]

Should we also warn about inline variables in anonymous namespaces?

clang/lib/Sema/SemaDecl.cpp

7127

This diagnostic should be independent of the ext / compat diagnostic below. This patch currently removes the -Wc++14-compat warning for static inline int n;, which would be a behavior regression for that warning.

Moving this check inside the else block a few lines below, and removing the check for CPlusPlus17, would seem reasonable. We accept inline variables in earlier language modes as an extension, and we should still warn in that case.

[serberoth]

Updates to testing to ensure warning occurs for variables declared in anonymous namespace.
Updates per rsmith to ensure that ext compat warning still occurs fixing regressive behaviour.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_static_assert_requirement_failed : Error<
   "static_assert failed due to requirement '%0'%select{ %2|}1">;
 
 def ext_inline_variable : ExtWarn<
   "inline variables are a C++17 extension">, InGroup<CXX17>;
 def warn_cxx14_compat_inline_variable : Warning<
   "inline variables are incompatible with C++ standards before C++17">,
   DefaultIgnore, InGroup<CXXPre17Compat>;
+def warn_cxx17_static_inline : Warning<
+  "variable %0 declared as both inline and with static storage declaration">,
+  InGroup<CXX17>;
+
 
 def warn_inline_namespace_reopened_noninline : Warning<
   "inline namespace reopened as a non-inline namespace">,
   InGroup<InlineNamespaceReopenedNoninline>;
 def err_inline_namespace_mismatch : Error<
   "non-inline namespace cannot be reopened as inline">;
 
 def err_unexpected_friend : Error<

clang/lib/Sema/SemaDecl.cpp

if (D.getDeclSpec().isInlineSpecified()) {
     if (!getLangOpts().CPlusPlus) {
       Diag(D.getDeclSpec().getInlineSpecLoc(), diag::err_inline_non_function)
           << 0;
     } else if (CurContext->isFunctionOrMethod()) {
       // 'inline' is not allowed on block scope variable declaration.
       Diag(D.getDeclSpec().getInlineSpecLoc(),
            diag::err_inline_declaration_block_scope) << Name
         << FixItHint::CreateRemoval(D.getDeclSpec().getInlineSpecLoc());
+    } else if (SC == SC_Static && DC->isFileContext() &&
+               getLangOpts().CPlusPlus17) {
+      // static inline declarations in the global or namespace scope should get
+      // a warning indicating they are contradictory in nature.
+      Diag(D.getIdentifierLoc(), diag::warn_cxx17_static_inline) << Name;
     } else {
       Diag(D.getDeclSpec().getInlineSpecLoc(),
            getLangOpts().CPlusPlus17 ? diag::warn_cxx14_compat_inline_variable
                                      : diag::ext_inline_variable);
       NewVD->setInlineSpecified();
     }
   }
 

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp

 // RUN: %clang_cc1 -std=c++1z -verify %s
 
 template<typename T> struct A { constexpr A(int = 0) {} };
 A() -> A<int>;
 A(int) -> A<char>;
 
-static constexpr inline const volatile A a = {}; // ok, specifiers are permitted
+static constexpr inline const volatile A a = {}; // expected-warning{{variable 'a' declared as both inline and with static storage declaration}}
 A b;
 A c [[]] {};
 
 A d = {}, e = {};
 A f(0), g{}; // expected-error {{template arguments deduced as 'A<char>' in declaration of 'f' and deduced as 'A<int>' in declaration of 'g'}}
 
 struct B {
   static A a; // expected-error {{requires an initializer}}
 };
 extern A x; // expected-error {{requires an initializer}}
 static A y;
 

clang/test/Parser/cxx1z-decomposition.cpp

struct S {
 
       // misc decl-specifiers
       friend auto &[g] = n; // expected-error {{'auto' not allowed}} expected-error {{friends can only be classes or functions}}
     };
     typedef auto &[h] = n; // expected-error {{cannot be declared 'typedef'}}
     constexpr auto &[i] = n; // expected-error {{cannot be declared 'constexpr'}}
   }
 
-  static constexpr inline thread_local auto &[j1] = n; // expected-error {{cannot be declared with 'constexpr inline' specifiers}}
+  static constexpr inline thread_local auto &[j1] = n; // expected-error {{cannot be declared with 'constexpr inline' specifiers}} expected-warning{{variable 'j1' declared as both inline and with static storage declaration}}
   static thread_local auto &[j2] = n; // expected-warning {{declared with 'static thread_local' specifiers is a C++20 extension}}
 
   inline auto &[k] = n; // expected-error {{cannot be declared 'inline'}}
 
   const int K = 5;
   void g() {
     // defining-type-specifiers other than cv-qualifiers and 'auto'
     S [a] = s; // expected-error {{cannot be declared with type 'BadSpecifiers::S'}}

clang/test/Sema/c17-global-static-inline.cpp

+// RUN: %clang_cc1 -std=c++17 %s -verify
+
+static inline int should_warn; // expected-warning{{variable 'should_warn' declared as both inline and with static storage declaration}}
+
+namespace TestNamespace {
+
+static inline int NamespacedShouldWarn; // expected-warning{{variable 'NamespacedShouldWarn' declared as both inline and with static storage declaration}}
+
+};
+
+static constexpr inline const volatile float f = 0.0f; // expected-warning{{variable 'f' declared as both inline and with static storage declaration}}
+
+static inline int TestFunction(int s, int e, int n) {
+  int m = (n - s) / (e - s);
+  return m * m * m * (m * (m * 6 - 15) + 10);
+}