[clang-tidy] Add option "LiteralInitializers" to cppcoreguidelines-pro-type-member-init

[mgehre]

Summary

Adds the option "LiteralInitializers" to
cppcoreguidelines-pro-type-member-init.
If set to non-zero, the check will provide fix-its with literal
initializers (`int i = 0;) instead of curly braces (int i{};`).

If find that more readable. Of course, this is purely a question of style,
thus the option is disabled by default.

[mgehre]

Extend test to negative case

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

377

What about cases where a suffix is desired, like U or LL?

379

Same here for f.

test/clang-tidy/cppcoreguidelines-pro-type-member-init-literal-initializers.cpp

14

The * should bind to ptr.

[hokein]

test/clang-tidy/cppcoreguidelines-pro-type-member-init-literal-initializers.cpp

1

-std=c++11 is not needed. This extra compile argument is added by default when running check_clang_tidy.

[mgehre]

Add suffix for floating point literals; clang-format

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

377

Is this really necessary for initializing to zero?
The compiler will figure out the correct thing, and I personally find "0" easier to read than "0ULL".

379

This is now fixed in the check.

test/clang-tidy/cppcoreguidelines-pro-type-member-init-literal-initializers.cpp

1

If I remove `-std=c++11`, the behavior changes and Context.getLangOpts().CPlusPlus11 is false.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

377

It is necessary because of the way literals are typed. [lex.icon]p2 lists what happens when there is no suffix on a literal, but it boils down to the compiler picking the first type that can represent the value, out of int, long int, or long long int. Without the U or the LL, the type of the literal will not match the type of the field. This shouldn't have an impact on the generated code, but it may cause diagnostics for some compilers.

[mgehre]

Put 'u' and 'l' on integer literals

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

303

type doesn't follow our usual naming conventions. I would recommend QT.

312

You shouldn't be calling an "internal" function here. Instead, you can do dyn_cast<BuiltinType>(type.getCanonicalType().getTypePtr())

324

Some users care deeply about l vs L because of how hard it is to distinguish depending on code fonts. I don't know that we need an option for that sort of thing, but my personal preference is to use capital letters rather than lowercase ones. (I originally read this as initializing to 0.01, FWIW).

test/clang-tidy/cppcoreguidelines-pro-type-member-init-literal-initializers.cpp

1

That's because the extra args you specify disable the c++11 specification, so this is correct to do.

[malcolm.parsons]

The modernize-use-default-member-init check now has an option with the same effect, but it is called UseAssignment.
We should use consistent option names.
Is there any way for multiple checks to share an option?

[alexfh]

In D24892#723536, @malcolm.parsons wrote:

The modernize-use-default-member-init check now has an option with the same effect, but it is called UseAssignment.
We should use consistent option names.
Is there any way for multiple checks to share an option?

There's OptionsView::getLocalOrGlobal. See how StrictMode option is read in ArgumentCommentCheck, for example.

[malcolm.parsons]

In D24892#732217, @alexfh wrote:

In D24892#723536, @malcolm.parsons wrote:

Is there any way for multiple checks to share an option?

There's OptionsView::getLocalOrGlobal. See how StrictMode option is read in ArgumentCommentCheck, for example.

ArgumentCommentCheck uses getLocalOrGlobal, but InefficientStringConcatenationCheck and SuspiciousEnumUsageCheck don't.
6 checks have an IncludeStyle option that isn't shared.
4 checks share a HeaderFileExtensions option, but with different defaults.

[alexfh]

In D24892#732243, @malcolm.parsons wrote:

In D24892#732217, @alexfh wrote:

In D24892#723536, @malcolm.parsons wrote:

Is there any way for multiple checks to share an option?

There's OptionsView::getLocalOrGlobal. See how StrictMode option is read in ArgumentCommentCheck, for example.

ArgumentCommentCheck uses getLocalOrGlobal, but InefficientStringConcatenationCheck and SuspiciousEnumUsageCheck don't.
6 checks have an IncludeStyle option that isn't shared.
4 checks share a HeaderFileExtensions option, but with different defaults.

The right thing would be to fix these. I might get around to this, if nobody does this before me.

[alexfh]

In D24892#733016, @alexfh wrote:

In D24892#732243, @malcolm.parsons wrote:

In D24892#732217, @alexfh wrote:

In D24892#723536, @malcolm.parsons wrote:

Is there any way for multiple checks to share an option?

There's OptionsView::getLocalOrGlobal. See how StrictMode option is read in ArgumentCommentCheck, for example.

ArgumentCommentCheck uses getLocalOrGlobal, but InefficientStringConcatenationCheck and SuspiciousEnumUsageCheck don't.
6 checks have an IncludeStyle option that isn't shared.
4 checks share a HeaderFileExtensions option, but with different defaults.

The right thing would be to fix these. I might get around to this, if nobody does this before me.

Better late than never: r308605 ;)

Any specific plans re: this patch?

[mgehre]

Implemented all review comments
Use global settings "UseAssignment"

[alexfh]

A few more nits.

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h

66

nit: Please add an empty line before the comment.

66

"use literals" and "UseAssignment" should be consistent, I guess.

test/clang-tidy/cppcoreguidelines-pro-type-member-init-use-assignment.cpp

38 ↗

(On Diff #107632)

No need for a semicolon here.

[mgehre]

Implement review comments

[aaron.ballman]

LGTM

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

361 ↗

(On Diff #198714)

We may not have to care about this for this version of the patch, but I believe we have other checks with options to add literal suffixes in lowercase vs uppercase form. We should try to honor that the same way we're honoring using assignment vs braces.

[mgehre]

Do I wait for @alexfh to turn his red into a green, too?

[aaron.ballman]

In D24892#1500371, @mgehre wrote:

Do I wait for @alexfh to turn his red into a green, too?

I think you covered all of the concerns he raised. If he doesn't give you an LG in the next 24 hours, I think you can go ahead and commit. If there are remaining issues, we can deal with them post-commit.

[lebedev.ri]

clang-tools-extra/docs/ReleaseNotes.rst

66–69 ↗

(On Diff #198714)

This is in the wrong category

[mgehre]

Fix wrong merge of ReleaseNotes

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h

private:
   // type without () or {} to initialize its members.
   //
   // To fix: Add () or {}.
   void checkUninitializedTrivialType(const ASTContext &Context,
                                      const VarDecl *Var);
 
   // Whether arrays need to be initialized or not. Default is false.
   bool IgnoreArrays;
+  // Whether fix-its for initializers of fundamental type use literals. Only
+  // effective in C++11 mode. Default is false.
+  bool LiteralInitializers;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_MEMBER_INIT_H

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

void fixInitializerList(const ASTContext &Context, DiagnosticBuilder &Diag,
   }
 }
 
 } // anonymous namespace
 
 ProTypeMemberInitCheck::ProTypeMemberInitCheck(StringRef Name,
                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      IgnoreArrays(Options.get("IgnoreArrays", false)) {}
+      IgnoreArrays(Options.get("IgnoreArrays", false)),
+      LiteralInitializers(Options.get("LiteralInitializers", false)) {}
 
 void ProTypeMemberInitCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   auto IsUserProvidedNonDelegatingConstructor =
       allOf(isUserProvided(),
             unless(anyOf(isInstantiated(), isDelegatingConstructor())));
if (const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor")) {
     checkMissingBaseClassInitializer(*Result.Context, Ctor);
   } else if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var")) {
     checkUninitializedTrivialType(*Result.Context, Var);
   }
 }
 
 void ProTypeMemberInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreArrays", IgnoreArrays);
+  Options.store(Opts, "LiteralInitializers", LiteralInitializers);
 }
 
 void ProTypeMemberInitCheck::checkMissingMemberInitializer(
     ASTContext &Context, const CXXConstructorDecl *Ctor) {
   const CXXRecordDecl *ClassDecl = Ctor->getParent();
   bool IsUnion = ClassDecl->isUnion();
 
   if (IsUnion && ClassDecl->hasInClassInitializer())
     return;
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet<const FieldDecl *, 16> FieldsToInit;
   forEachField(ClassDecl, ClassDecl->fields(), false, [&](const FieldDecl *F) {
     if (!F->hasInClassInitializer() &&
         utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
                                                             Context))
       FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
     return;
 
   for (const CXXCtorInitializer *Init : Ctor->inits()) {
     // Remove any fields that were explicitly written in the initializer list
     // or in-class.
     if (Init->isAnyMemberInitializer() && Init->isWritten()) {
       if (IsUnion)
         return; // We can only initialize one member of a union.
       FieldsToInit.erase(Init->getAnyMember());
     }
   }
   removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
 
if (!F->getType()->isEnumeralType())
       FieldsToFix.insert(F);
   });
   if (FieldsToFix.empty())
     return;
 
   // Use in-class initialization if possible.
   if (Context.getLangOpts().CPlusPlus11) {
     for (const FieldDecl *Field : FieldsToFix) {
+      QualType type = Field->getType();
+      const char *Initializer = "{}";
+      if (LiteralInitializers) {
+        if (type->isBooleanType())
+          Initializer = " = false";
+        else if (type->isIntegerType())
+          Initializer = " = 0";
+        else if (type->isFloatingType())
+          Initializer = " = 0.0";
+        else if (type->isPointerType())
+          Initializer = " = nullptr";
+      }
       Diag << FixItHint::CreateInsertion(
           getLocationForEndOfToken(Context, Field->getSourceRange().getEnd()),
-          "{}");
+          Initializer);
     }
   } else {
     // Otherwise, rewrite the constructor's initializer list.
     fixInitializerList(Context, Diag, Ctor, FieldsToFix);
   }
 }
 
 void ProTypeMemberInitCheck::checkMissingBaseClassInitializer(

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

 -------
 
 .. option:: IgnoreArrays
 
    If set to non-zero, the check will not warn about array members that are not
    zero-initialized during construction. For performance critical code, it may
    be important to not initialize fixed-size array members.  Default is `0`.
 
+.. option:: LiteralInitializers
+   If set to non-zero, the check will provide fix-its with literal initializers
+   (``int i = 0;``) instead of curly braces (``int i{};``).
+
 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.

test/clang-tidy/cppcoreguidelines-pro-type-member-init-literal-initializers.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t -- -config="{CheckOptions: [{key: "cppcoreguidelines-pro-type-member-init.LiteralInitializers", value: 1}]}" -- -std=c++11
+
+struct T {
+  int i;
+};
+
+struct S {
+  bool b;
+  // CHECK-FIXES: bool b = false;
+  int i;
+  // CHECK-FIXES: int i = 0;
+  float f;
+  // CHECK-FIXES: float f = 0.0;
+  int* ptr;
+  // CHECK-FIXES: int* ptr = nullptr;
+  T t;
+  // CHECK-FIXES: T t{};
+  S() {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields:
+};
+