Create diagnostic group for definition deprecation warning

[nuriamari]

Summary

In https://reviews.llvm.org/D126664, a warning is introduced
warning against the deprecated out of line definition of a
static constexpr member in C++17 and later. Prior to this patch,
the only diagnostic group controlling this diagnostic was -Wdeprecated,
which controls many many diagnostics. This patch creates
a diagnostic group specifically for this warning so it can
be controlled in isolation, while also being included with -Wdeprecated.

[nuriamari]

clang/test/CXX/basic/basic.def/p3.cpp

1 ↗

(On Diff #534993)

I'm not familiar with the naming convention of these test files. https://reviews.llvm.org/D126664 included a change in p2.cpp, and there is also a p4.cpp in the same directory, so I somewhat arbitrarily named this test p3.cpp. If these are meant to somehow correspond to the C++ standard or similar, please let me know.

[aaron.ballman]

I think it's a bit odd that we'd leave const under -Wdeprecated but separate constexpr out into its own warning flag, but I'm also not opposed. Can you explain the need a bit more though? I think our belief was that silencing this diagnostic was pretty trivial (delete the line in question), so we wouldn't need a separate diagnostic group for it.

Also, these changes should have a release note added to clang/docs/ReleaseNotes.rst.

clang/test/CXX/basic/basic.def/p3.cpp

1 ↗

(On Diff #534993)

Tests that live in clang/test/CXX/ or clang/test/C are for testing specific details of the standard specification. In C, we track based on which N-numbered document the feature was proposed in, and in C++ we track based on stable name (basic/basic.def) and paragraph number (p2.cpp) of the changes being tested.

I think your new test should live in clang/test/SemaCXX/ as that's where we put tests for more general compiler behaviors like warning flags.

[nuriamari]

In D153881#4467127, @aaron.ballman wrote:

I think it's a bit odd that we'd leave const under -Wdeprecated but separate constexpr out into its own warning flag, but I'm also not opposed.

Would moving both const and constexpr into their own warning flag work? I don't really see cases where you'd want to disable only one or the other.

Can you explain the need a bit more though? I think our belief was that silencing this diagnostic was pretty trivial (delete the line in question), so we wouldn't need a separate diagnostic group for it.

In our case we have libraries that are consumed both with say C++14 and C++17, so we can't just delete them, we've needed to add a standard version check. Admittedly not a huge change either, but we ran into many occurrences of this warning trying to adopt the latest Clang. I suppose I don't see the downside of a little more granular control.

Also, these changes should have a release note added to clang/docs/ReleaseNotes.rst.

Will do.

[aaron.ballman]

In D153881#4468597, @nuriamari wrote:

In D153881#4467127, @aaron.ballman wrote:

I think it's a bit odd that we'd leave const under -Wdeprecated but separate constexpr out into its own warning flag, but I'm also not opposed.

Would moving both const and constexpr into their own warning flag work? I don't really see cases where you'd want to disable only one or the other.

I think that would work better, yes.

Can you explain the need a bit more though? I think our belief was that silencing this diagnostic was pretty trivial (delete the line in question), so we wouldn't need a separate diagnostic group for it.

In our case we have libraries that are consumed both with say C++14 and C++17, so we can't just delete them, we've needed to add a standard version check. Admittedly not a huge change either, but we ran into many occurrences of this warning trying to adopt the latest Clang. I suppose I don't see the downside of a little more granular control.

Ah, I see now, thank you. Backcompat would be harder without this kind of granular control, so I'm okay with this direction.

[nuriamari]

• Create entry in release notes
• Move test to appropriate location
• Expand test to include const warning controlled in the same warning group

[nuriamari]

clang/docs/ReleaseNotes.rst

380

Happy to use a better flag name, if anyone has ideas. Considered expanding to be more descriptive, but its already quite long.

clang/test/SemaCXX/redundant-out-of-line-static-constexpr-member-def-diag.cpp

10

Unbeknownst to me, the const variant of this diagnostic (still about a static constexpr member, but defined out-of-line with const) is already controlled via the same diagnostic group. Is this the behavior you expect @aaron.ballman?

[aaron.ballman]

LGTM! Do you need me to land this on your behalf? If so, what name and email address would you like me to use for patch attribution? (If I'm landing it, I can fix up the release note for you when landing.)

clang/docs/ReleaseNotes.rst

380–383

Reworded a bit

clang/test/SemaCXX/redundant-out-of-line-static-constexpr-member-def-diag.cpp

10

Oh, nice! I wasn't aware we already covered that under the same group.

[nuriamari]

In D153881#4497041, @aaron.ballman wrote:

LGTM! Do you need me to land this on your behalf? If so, what name and email address would you like me to use for patch attribution? (If I'm landing it, I can fix up the release note for you when landing.)

I do. Nuri Amari - [e-mail] would be great, thanks.

clang/docs/ReleaseNotes.rst

 - The Fix-It emitted for unused labels used to expand to the next line, which caused
   visual oddities now that Clang shows more than one line of code snippet. This has
   been fixed and the Fix-It now only spans to the end of the ``:``.
 - Clang now underlines the parameter list of function declaration when emitting
   a note about the mismatch in the number of arguments.
 - Clang now diagnoses unexpected tokens after a
   ``#pragma clang|GCC diagnostic push|pop`` directive.
   (`#13920: <https://github.com/llvm/llvm-project/issues/13920>`_)
 - Clang now does not try to analyze cast validity on variables with dependent alignment (`#63007: <https://github.com/llvm/llvm-project/issues/63007>`_).
 - Clang constexpr evaluator now displays member function calls more precisely
   by making use of the syntactical structure of function calls. This avoids display
   of syntactically invalid codes in diagnostics.
   (`#57081: <https://github.com/llvm/llvm-project/issues/57081>`_)
 - Clang no longer emits inappropriate notes about the loss of ``__unaligned`` qualifier
   on overload resolution, when the actual reason for the failure is loss of other qualifiers.
 - Clang's notes about unconvertible types in overload resolution failure now covers
   the source range of parameter declaration of the candidate function declaration.
+- Added a new diagnostic warning group
+  ``-Wdeprecated-redundant-constexpr-static-def``, under the existing
+  ``-Wdeprecated`` group. This controls warnings about out-of-line definitions
+  of 'static constexpr' data members that are unnecessary from C++17 onwards.
+
+ Bug Fixes in This Version
+ -------------------------
 
   *Example Code*:
 
   .. code-block:: c++
 
      void func(int aa, int bb);
      void test() { func(1, "two"); }
 

clang/include/clang/Basic/DiagnosticGroups.td

 def DeprecatedCommaSubscript : DiagGroup<"deprecated-comma-subscript">;
 def DeprecatedCopyWithUserProvidedCopy : DiagGroup<"deprecated-copy-with-user-provided-copy">;
 def DeprecatedCopyWithUserProvidedDtor : DiagGroup<"deprecated-copy-with-user-provided-dtor">;
 def DeprecatedCopy : DiagGroup<"deprecated-copy", [DeprecatedCopyWithUserProvidedCopy]>;
 def DeprecatedCopyWithDtor : DiagGroup<"deprecated-copy-with-dtor", [DeprecatedCopyWithUserProvidedDtor]>;
 // For compatibility with GCC.
 def : DiagGroup<"deprecated-copy-dtor", [DeprecatedCopyWithDtor]>;
 def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
+def DeprecatedRedundantConstexprStaticDef : DiagGroup<"deprecated-redundant-constexpr-static-def">;
 def UnavailableDeclarations : DiagGroup<"unavailable-declarations">;
 def UnguardedAvailabilityNew : DiagGroup<"unguarded-availability-new">;
 def UnguardedAvailability : DiagGroup<"unguarded-availability",
                                       [UnguardedAvailabilityNew]>;
 // partial-availability is an alias of unguarded-availability.
 def : DiagGroup<"partial-availability", [UnguardedAvailability]>;
 def DeprecatedDynamicExceptionSpec
     : DiagGroup<"deprecated-dynamic-exception-spec">;
def Deprecated : DiagGroup<"deprecated", [DeprecatedAnonEnumEnumConversion,
                                           DeprecatedEnumFloatConversion,
                                           DeprecatedBuiltins,
                                           DeprecatedIncrementBool,
                                           DeprecatedPragma,
                                           DeprecatedRegister,
                                           DeprecatedThisCapture,
                                           DeprecatedType,
                                           DeprecatedVolatile,
-                                          DeprecatedWritableStr]>,
+                                          DeprecatedWritableStr,
+                                          DeprecatedRedundantConstexprStaticDef,
+                                          ]>,
                  DiagCategory<"Deprecations">;
 
 def CXX20Designator : DiagGroup<"c++20-designator">;
 // Allow -Wno-c99-designator to be used to turn off all warnings on valid C99
 // designators (including the warning controlled by -Wc++20-designator).
 def C99Designator : DiagGroup<"c99-designator", [CXX20Designator]>;
 def GNUDesignator : DiagGroup<"gnu-designator">;
 def DtorName : DiagGroup<"dtor-name">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_noreturn_non_function : Error<
   "'_Noreturn' can only appear on functions">;
 def warn_qual_return_type : Warning<
   "'%0' type qualifier%s1 on return type %plural{1:has|:have}1 no effect">,
   InGroup<IgnoredQualifiers>, DefaultIgnore;
 def warn_deprecated_redundant_constexpr_static_def : Warning<
   "out-of-line definition of constexpr static data member is redundant "
   "in C++17 and is deprecated">,
-  InGroup<Deprecated>, DefaultIgnore;
+  InGroup<DeprecatedRedundantConstexprStaticDef>, DefaultIgnore;
 
 def warn_decl_shadow :
   Warning<"declaration shadows a %select{"
           "local variable|"
           "variable in %2|"
           "static data member of %2|"
           "field of %2|"
           "typedef in %2|"

clang/test/SemaCXX/redundant-out-of-line-static-constexpr-member-def-diag.cpp

+// RUN: %clang_cc1 -std=c++17 -verify %s -Werror -Wdeprecated -Wno-error=deprecated-redundant-constexpr-static-def
+
+namespace {
+  struct A {
+    static constexpr int n = 0;
+    static constexpr int m = 0;
+  };
+  constexpr int A::n; // expected-warning{{out-of-line definition of constexpr static data member is redundant in C++17 and is deprecated}}
+  const int A::m; // expected-warning{{out-of-line definition of constexpr static data member is redundant in C++17 and is deprecated}}
+}