[PATCH] D102663: Bug 49633 - Added warning for static inline global and namespaced declarations for c++17+

[Serberoth via Phabricator via cfe-commits]

serberoth created this revision.
serberoth added reviewers: erik.pilkington, rsmith, TH3CHARLie, fixing bugs in 
llvm.
serberoth requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Bug 49633 - Added a warning for global and namespace declared variables that 
are declared as both static and inline when using C++17 and above.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102663

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
  clang/test/Parser/cxx1z-decomposition.cpp
  clang/test/Sema/c17-global-static-inline.cpp


Index: clang/test/Sema/c17-global-static-inline.cpp
===
--- /dev/null
+++ clang/test/Sema/c17-global-static-inline.cpp
@@ -0,0 +1,16 @@
+// 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);
+}
Index: clang/test/Parser/cxx1z-decomposition.cpp
===
--- clang/test/Parser/cxx1z-decomposition.cpp
+++ clang/test/Parser/cxx1z-decomposition.cpp
@@ -84,7 +84,7 @@
 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'}}
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
@@ -4,7 +4,7 @@
 A() -> A;
 A(int) -> A;
 
-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 [[]] {};
 
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7124,6 +7124,11 @@
   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
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1466,6 +1466,10 @@
 def warn_cxx14_compat_inline_variable : Warning<
   "inline variables are incompatible with C++ standards before C++17">,
   DefaultIgnore, InGroup;
+def warn_cxx17_static_inline : Warning<
+  "variable %0 declared as both inline and with static storage declaration">,
+  InGroup;
+
 
 def warn_inline_namespace_reopened_noninline : Warning<
   "inline namespace reopened as a non-inline namespace">,


Index: clang/test/Sema/c17-global-static-inline.cpp
===
--- /dev/null
+++ clang/test/Sema/c17-global-static-inline.cpp
@@ -0,0 +1,16 @@
+// 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 NamespacedSho

[PATCH] D102663: Bug 49633 - Added warning for static inline global and namespaced declarations for c++17+

[Serberoth via Phabricator via cfe-commits]

serberoth added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:7127
 << FixItHint::CreateRemoval(D.getDeclSpec().getInlineSpecLoc());
+} else if (SC == SC_Static && DC->isFileContext() &&
+   getLangOpts().CPlusPlus17) {

erichkeane wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102663/new/

https://reviews.llvm.org/D102663

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102663: Bug 49633 - Added warning for static inline global and namespaced declarations

[Serberoth via Phabricator via cfe-commits]

serberoth updated this revision to Diff 348156.
serberoth retitled this revision from "Bug 49633 - Added warning for static 
inline global and namespaced declarations for c++17+" to "Bug 49633 - Added 
warning for static inline global and namespaced declarations".
serberoth edited the summary of this revision.
serberoth added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102663/new/

https://reviews.llvm.org/D102663

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
  clang/test/Parser/cxx1z-decomposition.cpp
  clang/test/Sema/c17-global-static-inline.cpp


Index: clang/test/Sema/c17-global-static-inline.cpp
===
--- /dev/null
+++ clang/test/Sema/c17-global-static-inline.cpp
@@ -0,0 +1,22 @@
+// 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}}
+
+};
+
+namespace {
+
+  static inline int AnonNamespaceShouldWarn; // expected-warning{{variable 
'AnonNamespaceShouldWarn' 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);
+}
Index: clang/test/Parser/cxx1z-decomposition.cpp
===
--- clang/test/Parser/cxx1z-decomposition.cpp
+++ clang/test/Parser/cxx1z-decomposition.cpp
@@ -84,7 +84,7 @@
 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'}}
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.class.deduct/p1.cpp
@@ -4,7 +4,7 @@
 A() -> A;
 A(int) -> A;
 
-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 [[]] {};
 
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7125,6 +7125,11 @@
diag::err_inline_declaration_block_scope) << Name
 << FixItHint::CreateRemoval(D.getDeclSpec().getInlineSpecLoc());
 } else {
+  // static inline declarations in the global or namespace scope should get
+  // a warning indicating they are contradictory in nature.
+  if (SC == SC_Static && DC->isFileContext())
+Diag(D.getIdentifierLoc(), diag::warn_static_inline) << Name;
+  
   Diag(D.getDeclSpec().getInlineSpecLoc(),
getLangOpts().CPlusPlus17 ? diag::warn_cxx14_compat_inline_variable
  : diag::ext_inline_variable);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1466,6 +1466,10 @@
 def warn_cxx14_compat_inline_variable : Warning<
   "inline variables are incompatible with C++ standards before C++17">,
   DefaultIgnore, InGroup;
+def warn_static_inline : Warning<
+  "variable %0 declared as both inline and with static storage declaration">,
+  InGroup;
+
 
 def warn_inline_namespace_reopened_noninline : Warning<
   "inline namespace reopened as a non-inline namespace">,


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