[clang-tidy] cppcoreguidelines-pro-type-member-init should not complain about static variables

[alexfh]

Summary

Variables with static storage duration are zero-initialized per
[stmt.dcl]p4 and [basic.start.init]p2.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

284 ↗

(On Diff #55441)

We should add a test that this still diagnoses variables with dynamic storage duration. Alternatively, this could be unless(anyOf(hasStaticStorageDuration(), hasThreadStorageDuration()))

[alexfh]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

284 ↗

(On Diff #55441)

I don't think it ever diagnosed on the dynamic storage duration, since it doesn't match cxxNewExpr ;)

[michael_miller]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

284 ↗

(On Diff #55441)

Perhaps unrelated but a new expression without the parens wouldn't value-initialize a record type with a default constructor and potentially be a problematic. Maybe we should be checking cxxNewExpr.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

284 ↗

(On Diff #55441)

Shouldn't it have diagnosed:

struct s {
  int *i;
  s() {}
};

?

[alexfh]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

284 ↗

(On Diff #55441)

This code does not introduce an object with dynamic storage duration. Variable i is just a member variable, and the fact that it's a pointer doesn't change much. The check used to warn in this case and continues to do so:

/tmp/q.cc:3:3: warning: constructor does not initialize these fields: i [cppcoreguidelines-pro-type-member-init]
  s() {}
  ^
      : i()

To resolve the confusion:

[basic.stc]p2

Static, thread, and automatic storage durations are associated with objects introduced by declarations (3.1) and implicitly created by the implementation (12.2). The dynamic storage duration is associated with objects created with operator new (5.3.4).

284 ↗

(On Diff #55441)

Michael, we should be checking cxxNewExpr, but I would leave this to you or someone else. In this patch I'm just trying to make the check less noisy.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

284 ↗

(On Diff #55441)

Ah, that's a very valid point.

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

Finder->addMatcher(
           .bind("ctor"),
       this);
   auto HasDefaultConstructor = hasInitializer(
       cxxConstructExpr(unless(requiresZeroInitialization()),
                        hasDeclaration(cxxConstructorDecl(
                            isDefaultConstructor(), unless(isUserProvided())))));
   Finder->addMatcher(
       varDecl(isDefinition(), HasDefaultConstructor,
+              hasAutomaticStorageDuration(),
               hasType(recordDecl(has(fieldDecl()),
                                  isTriviallyDefaultConstructible())))
           .bind("var"),
       this);
 }
 
 void ProTypeMemberInitCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor")) {

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst

 older versions it inserts the field initializers into the constructor
 initializer list. It will also initialize any direct base classes that
 need to be zeroed in the constructor initializer list.
 
 The check takes assignment of fields in the constructor body into
 account but generates false positives for fields initialized in
 methods invoked in the constructor body.
 
-The check also flags variables of record types without a user-provided
-constructor that are not initialized. The suggested fix is to zero
-initialize the variable via {} for C++11 and beyond or = {} for older
-versions.
+The check also flags variables with automatic storage duration that have record
+types without a user-provided constructor and are not initialized. The suggested
+fix is to zero initialize the variable via ``{}`` for C++11 and beyond or ``=
+{}`` for older language versions.
 
 IgnoreArrays option
 -------------------
 
 For performance critical code, it may be important to not zero
 fixed-size array members. If on, IgnoreArrays will not warn about
 array members that are not zero-initialized during construction.
 IgnoreArrays is false by default.
 
 This rule is part of the "Type safety" profile of the C++ Core
 Guidelines, corresponding to rule Type.6. See
 https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-type-memberinit.

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp

struct ComplexNonTrivialType {
   int Y;
   // CHECK-FIXES: int Y{};
 };
 
 static void PositiveComplexNonTrivialType() {
   ComplexNonTrivialType T;
 }
 
-struct PositiveStaticMember {
+struct NegativeStaticMember {
   static NonTrivialType X;
   static NonTrivialType Y;
   static constexpr NonTrivialType Z{};
 };
 
-NonTrivialType PositiveStaticMember::X;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: uninitialized record type: 'X'
-// CHECK-FIXES: NonTrivialType PositiveStaticMember::X{};
-
-NonTrivialType PositiveStaticMember::Y{};
+NonTrivialType NegativeStaticMember::X;
+NonTrivialType NegativeStaticMember::Y{};
 
 struct PositiveMultipleConstructors {
   PositiveMultipleConstructors() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B
 
   PositiveMultipleConstructors(int) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B
 

 struct NegativeUninitializedBase : public PositiveUninitializedBase {
   NegativeUninitializedBase() {}
 };
 
 struct InheritedAggregate : public NegativeAggregateType {
   int F;
 };
 
-static InheritedAggregate PositiveGlobal;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: uninitialized record type: 'PositiveGlobal'
-// CHECK-FIXES: InheritedAggregate PositiveGlobal{};
+static InheritedAggregate NegativeGlobal;
 
 enum TestEnum {
   A,
   B,
   C
 };
 
 enum class TestScopedEnum {

 
 static void PositiveCStructVariable() {
   NegativeCStruct X;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: uninitialized record type: 'X'
   // CHECK-FIXES: NegativeCStruct X{};
 }
 }
 
+static void NegativeStaticVariable() {
+  static NegativeCStruct S;
+  (void)S;
+}
+
 union NegativeUnionInClass {
   NegativeUnionInClass() {} // No message as a union can only initialize one member.
   int X = 0;
   float Y;
 };
 
 union PositiveUnion {
   PositiveUnion() : X() {} // No message as a union can only initialize one member.