[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 ↗

(On Diff #72394)

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

379 ↗

(On Diff #72394)

Same here for f.

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

14 ↗

(On Diff #72394)

The * should bind to ptr.

[hokein]

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

1 ↗

(On Diff #72394)

-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 ↗

(On Diff #72394)

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 ↗

(On Diff #72394)

This is now fixed in the check.

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

1 ↗

(On Diff #72394)

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

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

377 ↗

(On Diff #72394)

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

348 ↗

(On Diff #93511)

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

357 ↗

(On Diff #93511)

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

369 ↗

(On Diff #93511)

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 ↗

(On Diff #72394)

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

68 ↗

(On Diff #107632)

nit: Please add an empty line before the comment.

68 ↗

(On Diff #107632)

"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-tools-extra/trunk/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 initialization of fundamental type use assignment
+  // instead of brace initalization. Only effective in C++11 mode. Default is
+  // false.
+  bool UseAssignment;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_MEMBER_INIT_H

clang-tools-extra/trunk/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)),
+      UseAssignment(Options.getLocalOrGlobal("UseAssignment", 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, *Record, nullptr);
   } 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, "UseAssignment", UseAssignment);
 }
 
 // FIXME: Copied from clang/lib/Sema/SemaDeclCXX.cpp.
 static bool isIncompleteOrZeroLengthArrayType(ASTContext &Context, QualType T) {
   if (T->isIncompleteArrayType())
     return true;
 
   while (const ConstantArrayType *ArrayT = Context.getAsConstantArrayType(T)) {
     if (!ArrayT->getSize())
       return true;
 
     T = ArrayT->getElementType();
   }
 
   return false;
 }
 
 static bool isEmpty(ASTContext &Context, const QualType &Type) {
   if (const CXXRecordDecl *ClassDecl = Type->getAsCXXRecordDecl()) {
     return ClassDecl->isEmpty();
   }
   return isIncompleteOrZeroLengthArrayType(Context, Type);
 }
 
+static const char *getInitializer(QualType QT, bool UseAssignment) {
+  const char *DefaultInitializer = "{}";
+  if (!UseAssignment)
+    return DefaultInitializer;
+
+  if (QT->isPointerType())
+    return " = nullptr";
+
+  const BuiltinType *BT =
+      dyn_cast<BuiltinType>(QT.getCanonicalType().getTypePtr());
+  if (!BT)
+    return DefaultInitializer;
+
+  switch (BT->getKind()) {
+  case BuiltinType::Bool:
+    return " = false";
+  case BuiltinType::Float:
+    return " = 0.0F";
+  case BuiltinType::Double:
+    return " = 0.0";
+  case BuiltinType::LongDouble:
+    return " = 0.0L";
+  case BuiltinType::SChar:
+  case BuiltinType::Char_S:
+  case BuiltinType::WChar_S:
+  case BuiltinType::Char16:
+  case BuiltinType::Char32:
+  case BuiltinType::Short:
+  case BuiltinType::Int:
+    return " = 0";
+  case BuiltinType::UChar:
+  case BuiltinType::Char_U:
+  case BuiltinType::WChar_U:
+  case BuiltinType::UShort:
+  case BuiltinType::UInt:
+    return " = 0U";
+  case BuiltinType::Long:
+    return " = 0L";
+  case BuiltinType::ULong:
+    return " = 0UL";
+  case BuiltinType::LongLong:
+    return " = 0LL";
+  case BuiltinType::ULongLong:
+    return " = 0ULL";
+
+  default:
+    return DefaultInitializer;
+  }
+}
+
 void ProTypeMemberInitCheck::checkMissingMemberInitializer(
     ASTContext &Context, const CXXRecordDecl &ClassDecl,
     const CXXConstructorDecl *Ctor) {
   bool IsUnion = ClassDecl.isUnion();
 
   if (IsUnion && ClassDecl.hasInClassInitializer())
     return;
 
void ProTypeMemberInitCheck::checkMissingMemberInitializer(
   if (FieldsToFix.empty())
     return;
 
   // Use in-class initialization if possible.
   if (Context.getLangOpts().CPlusPlus11) {
     for (const FieldDecl *Field : FieldsToFix) {
       Diag << FixItHint::CreateInsertion(
           getLocationForEndOfToken(Context, Field->getSourceRange().getEnd()),
-          "{}");
+          getInitializer(Field->getType(), UseAssignment));
     }
   } else if (Ctor) {
     // Otherwise, rewrite the constructor's initializer list.
     fixInitializerList(Context, Diag, Ctor, FieldsToFix);
   }
 }
 
 void ProTypeMemberInitCheck::checkMissingBaseClassInitializer(

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New :doc:`modernize-use-trailing-return-type
 
   Rewrites function signatures to use a trailing return type.
 
 - The :doc:`misc-throw-by-value-catch-by-reference
   <clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst>` now supports
   `WarnOnLargeObject` and `MaxSize` options to warn on any large trivial
   object caught by value.
 
+- Added `UseAssignment` option to :doc:`cppcoreguidelines-pro-type-member-init`
+
+  If set to true, the check will provide fix-its with literal initializers
+  (``int i = 0;``) instead of curly braces (``int i{};``).
+
 Improvements to include-fixer
 -----------------------------
 
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes
   out of the Structured Block it was thrown in.

clang-tools-extra/trunk/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:: UseAssignment
+   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.

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

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t -- -config="{CheckOptions: [{key: "cppcoreguidelines-pro-type-member-init.UseAssignment", value: 1}]}" -- -std=c++11
+
+struct T {
+  int i;
+};
+
+struct S {
+  bool b;
+  // CHECK-FIXES: bool b = false;
+  char c;
+  // CHECK-FIXES: char c = 0;
+  signed char sc;
+  // CHECK-FIXES: signed char sc = 0;
+  unsigned char uc;
+  // CHECK-FIXES: unsigned char uc = 0U;
+  int i;
+  // CHECK-FIXES: int i = 0;
+  unsigned u;
+  // CHECK-FIXES: unsigned u = 0U;
+  long l;
+  // CHECK-FIXES: long l = 0L;
+  unsigned long ul;
+  // CHECK-FIXES: unsigned long ul = 0UL;
+  long long ll;
+  // CHECK-FIXES: long long ll = 0LL;
+  unsigned long long ull;
+  // CHECK-FIXES: unsigned long long ull = 0ULL;
+  float f;
+  // CHECK-FIXES: float f = 0.0F;
+  double d;
+  // CHECK-FIXES: double d = 0.0;
+  long double ld;
+  // CHECK-FIXES: double ld = 0.0L;
+  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:
+};