[clang-tidy] Omit std::make_unique/make_shared for default initialization.

[ckennelly]

Summary

This extends the check for default initialization in arrays added in
547f89d6070 to include scalar types and exclude them from the suggested fix for
make_unique/make_shared.

Rewriting std::unique_ptr<int>(new int) as std::make_unique<int>() (or for
other, similar trivial T) switches from default initialization to value
initialization, a performance regression for trivial T. For these use cases,
std::make_unique_for_overwrite is more suitable alternative.

[ymandel]

clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp

135

No braces around the body.

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp

54

Please extend comment to briefly explain why there's no fix for this case.

[njames93]

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp

132–135

Drop the const, we don't use const on local variables in the codebase.

[ckennelly]

Apply feedback from ymandel

[ckennelly]

Apply review feedback from njames93

[ckennelly]

In D90392#2362118, @njames93 wrote:

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

I disagree with printing a warning but not a fix.

These uses should migrate to std::make_unique_for_overwrite/std::make_shared_for_overwrite. I am planning on sending a follow-up patch for that but want to avoid the existing make-unique/make-shared checks try to migrate default initialization use cases.

[lebedev.ri]

In D90392#2362308, @ckennelly wrote:

In D90392#2362118, @njames93 wrote:

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

I disagree with printing a warning but not a fix.

These uses should migrate to std::make_unique_for_overwrite/std::make_shared_for_overwrite. I am planning on sending a follow-up patch for that but want to avoid the existing make-unique/make-shared checks try to migrate default initialization use cases.

Reminder that there is more than one version of C++ standard, and users are not obligated to be using some particular version, and the checks should not be making that decision for user.

[ckennelly]

In D90392#2362327, @lebedev.ri wrote:

In D90392#2362308, @ckennelly wrote:

In D90392#2362118, @njames93 wrote:

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

I disagree with printing a warning but not a fix.

These uses should migrate to std::make_unique_for_overwrite/std::make_shared_for_overwrite. I am planning on sending a follow-up patch for that but want to avoid the existing make-unique/make-shared checks try to migrate default initialization use cases.

Reminder that there is more than one version of C++ standard, and users are not obligated to be using some particular version, and the checks should not be making that decision for user.

Understood, but replacing std::unique_ptr<int>(new int) with std::make_unique<int>() does change program behavior. Once the code migrates to value initialization, it can be hard to determine later whether that initialization ends up being unnecessary when C++20 or an analogue for std::make_unique_for_overwrite is available.

I'm hesitant to nudge users towards the behavior change when alternatives would be available (maintain the status quo, have C++20, preadopt a make_unique_for_overwrite helper like we do with this clang-tidy for pre-C++14 builds).

[ckennelly]

Ping

[lebedev.ri]

I think for starters this patch needs a better description, explaining not *what* it does, but *why* it does what it does.
Also, it is probably not good to change (break) existing test coverage.
Instead, it is best to add new tests, and adjust existing check lines.

[ckennelly]

Expand tests to cover value and default initialization

[steveire]

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp

45

I'm a bit confused. Why don't we want to transform this?

[ckennelly]

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp

45

std::make_shared<int>() is equivalent to std::shared_ptr<int>(new int()). This value initializes (read: zeroes out the value).

The statement here is only default initializing (read: leaves the value uninitialized). For this use case, we should use std::make_shared_for_overwrite when it is available.

[steveire]

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp

45

I see. Did you consider an option to transform these anyway? It seems at least as likely that for real-world code bases they are not initialized accidentally (as opposed to being a deliberate choice as assumed here).

[ckennelly]

njames93: Ping

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp

45

The default initialization case for arrays was added because it caused several performance regressions. I want to get the complete initialization check implemented, which will then make the make_unique_for_overwrite rewrite straight forward (just !initializating).

An option could then be added in a subsequent commit.

[lebedev.ri]

In D90392#2362118, @njames93 wrote:

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

+1

[ckennelly]

Added option to allow rewrites for default to value initialization

[ckennelly]

Ping

[ckennelly]

Ping

[lebedev.ri]

I think the patches description still could use some more words,
in particular it still only repeats what the patch does,
but not why it does that.

[hokein]

thanks, I think this patch is good, feel free to land it (and sorry for the delay).

As @lebedev.ri commented, would be nice to mention the motivation of the change (make_shared_for_overwrite?) in the description.

[ckennelly]

rebase

[ckennelly]

updating commit message

[ckennelly]

In D90392#2436639, @hokein wrote:

thanks, I think this patch is good, feel free to land it (and sorry for the delay).

As @lebedev.ri commented, would be nice to mention the motivation of the change (make_shared_for_overwrite?) in the description.

Done

[ckennelly]

@njames93: Any additional comments?

clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp

 //===--- MakeSmartPtrCheck.cpp - clang-tidy--------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "../utils/TypeTraits.h"
 #include "MakeSharedCheck.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
void MakeSmartPtrCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Reset = Result.Nodes.getNodeAs<CXXMemberCallExpr>(ResetCall);
   const auto *Type = Result.Nodes.getNodeAs<QualType>(PointerType);
   const auto *New = Result.Nodes.getNodeAs<CXXNewExpr>(NewExpression);
 
   // Skip when this is a new-expression with `auto`, e.g. new auto(1)
   if (New->getType()->getPointeeType()->getContainedAutoType())
     return;
 
-  // Be conservative for cases where we construct an array without any
-  // initialization.
+  // Be conservative for cases where we construct and default initialize.
+  //
   // For example,
+  //    P.reset(new int)    // check fix: P = std::make_unique<int>()
   //    P.reset(new int[5]) // check fix: P = std::make_unique<int []>(5)
   //
-  // The fix of the check has side effect, it introduces default initialization
+  // The fix of the check has side effect, it introduces value initialization
   // which maybe unexpected and cause performance regression.
-  if (New->isArray() && !New->hasInitializer())
+  const bool Initializes = New->hasInitializer() ||
+                           !utils::type_traits::isTriviallyDefaultConstructible(
+                               New->getAllocatedType(), *Result.Context);
+  if (!Initializes)
     return;
   if (Construct)
     checkConstruct(SM, Result.Context, Construct, Type, New);
   else if (Reset)
     checkReset(SM, Result.Context, Reset, New);
 }
 
 void MakeSmartPtrCheck::checkConstruct(SourceManager &SM, ASTContext *Ctx,

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp

std::shared_ptr<Base> getPointer() {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use std::make_shared instead
   // CHECK-FIXES: return std::make_shared<Base>();
 }
 
 void basic() {
   std::shared_ptr<int> P1 = std::shared_ptr<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: std::shared_ptr<int> P1 = std::make_shared<int>();
 
   P1.reset(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: P1 = std::make_shared<int>();
 
   P1 = std::shared_ptr<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: P1 = std::make_shared<int>();
 
-  // Without parenthesis.
+  // Without parenthesis, default initialization.
   std::shared_ptr<int> P2 = std::shared_ptr<int>(new int);
-  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_shared instead [modernize-make-shared]
-  // CHECK-FIXES: std::shared_ptr<int> P2 = std::make_shared<int>();
 
-  P2.reset(new int);
+  P2.reset(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: P2 = std::make_shared<int>();
 
-  P2 = std::shared_ptr<int>(new int);
+  P2 = std::shared_ptr<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: P2 = std::make_shared<int>();
 
   // With auto.
   auto P3 = std::shared_ptr<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use std::make_shared instead
   // CHECK-FIXES: auto P3 = std::make_shared<int>();
 
-  std::shared_ptr<int> P4 = std::shared_ptr<int>((new int));
+  std::shared_ptr<int> P4 = std::shared_ptr<int>((new int()));
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: std::shared_ptr<int> P4 = std::make_shared<int>();
 
   P4.reset((((new int()))));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: P4 = std::make_shared<int>();
 
-  P4 = std::shared_ptr<int>(((new int)));
+  P4 = std::shared_ptr<int>(((new int())));
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use std::make_shared instead [modernize-make-shared]
   // CHECK-FIXES: P4 = std::make_shared<int>();
 
   {
     // No std.
     using namespace std;
     shared_ptr<int> Q = shared_ptr<int>(new int());
     // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use std::make_shared instead
void initialization(int T, Base b) {
   // Aggregate initialization without arguments.
   std::shared_ptr<Empty> PEmpty = std::shared_ptr<Empty>(new Empty{});
   // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: use std::make_shared instead
   // CHECK-FIXES: std::shared_ptr<Empty> PEmpty = std::make_shared<Empty>(Empty{});
 }
 
 void aliases() {
   typedef std::shared_ptr<int> IntPtr;
-  IntPtr Typedef = IntPtr(new int);
+  IntPtr Typedef = IntPtr(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use std::make_shared instead
   // CHECK-FIXES: IntPtr Typedef = std::make_shared<int>();
 
   // We use 'bool' instead of '_Bool'.
   typedef std::shared_ptr<bool> BoolPtr;
-  BoolPtr BoolType = BoolPtr(new bool);
+  BoolPtr BoolType = BoolPtr(new bool());
   // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use std::make_shared instead
   // CHECK-FIXES: BoolPtr BoolType = std::make_shared<bool>();
 
   // We use 'Base' instead of 'struct Base'.
   typedef std::shared_ptr<Base> BasePtr;
   BasePtr StructType = BasePtr(new Base);
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
 // CHECK-FIXES: BasePtr StructType = std::make_shared<Base>();
 
 #define PTR shared_ptr<int>
-  std::shared_ptr<int> Macro = std::PTR(new int);
+  std::shared_ptr<int> Macro = std::PTR(new int());
 // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: use std::make_shared instead
 // CHECK-FIXES: std::shared_ptr<int> Macro = std::make_shared<int>();
 #undef PTR
 
-  std::shared_ptr<int> Using = shared_ptr_<int>(new int);
+  std::shared_ptr<int> Using = shared_ptr_<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: use std::make_shared instead
   // CHECK-FIXES: std::shared_ptr<int> Using = std::make_shared<int>();
 }
 
 void whitespaces() {
   // clang-format off
   auto Space = std::shared_ptr <int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use std::make_shared instead
   // CHECK-FIXES: auto Space = std::make_shared<int>();
 
   auto Spaces = std  ::    shared_ptr  <int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use std::make_shared instead
   // CHECK-FIXES: auto Spaces = std::make_shared<int>();
   // clang-format on
 }
 
 void nesting() {
   auto Nest = std::shared_ptr<std::shared_ptr<int>>(new std::shared_ptr<int>(new int));
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use std::make_shared instead
   // CHECK-FIXES: auto Nest = std::make_shared<std::shared_ptr<int>>(new int);
   Nest.reset(new std::shared_ptr<int>(new int));
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use std::make_shared instead
   // CHECK-FIXES: Nest = std::make_shared<std::shared_ptr<int>>(new int);
-  Nest->reset(new int);
+  Nest->reset(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use std::make_shared instead
   // CHECK-FIXES: *Nest = std::make_shared<int>();
 }
 
 void reset() {
   std::shared_ptr<int> P;
   P.reset();
   P.reset(nullptr);

clang-tools-extra/test/clang-tidy/checkers/modernize-make-unique.cpp

void basic() {
   P1.reset(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_unique instead [modernize-make-unique]
   // CHECK-FIXES: P1 = std::make_unique<int>();
 
   P1 = std::unique_ptr<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use std::make_unique instead [modernize-make-unique]
   // CHECK-FIXES: P1 = std::make_unique<int>();
 
-  // Without parenthesis.
+  // Without parenthesis, default initialization.
   std::unique_ptr<int> P2 = std::unique_ptr<int>(new int);
-  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_unique instead [modernize-make-unique]
-  // CHECK-FIXES: std::unique_ptr<int> P2 = std::make_unique<int>();
 
   P2.reset(new int);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_unique instead [modernize-make-unique]
-  // CHECK-FIXES: P2 = std::make_unique<int>();
 
   P2 = std::unique_ptr<int>(new int);
-  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use std::make_unique instead [modernize-make-unique]
-  // CHECK-FIXES: P2 = std::make_unique<int>();
 
   // With auto.
   auto P3 = std::unique_ptr<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use std::make_unique instead
   // CHECK-FIXES: auto P3 = std::make_unique<int>();
 
-  std::unique_ptr<int> P4 = std::unique_ptr<int>((new int));
+  std::unique_ptr<int> P4 = std::unique_ptr<int>((new int()));
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_unique instead [modernize-make-unique]
   // CHECK-FIXES: std::unique_ptr<int> P4 = std::make_unique<int>();
-  P4.reset((new int));
+  P4.reset((new int()));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_unique instead [modernize-make-unique]
   // CHECK-FIXES: P4 = std::make_unique<int>();
-  std::unique_ptr<int> P5 = std::unique_ptr<int>((((new int))));
+  std::unique_ptr<int> P5 = std::unique_ptr<int>((((new int()))));
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_unique instead [modernize-make-unique]
   // CHECK-FIXES: std::unique_ptr<int> P5 = std::make_unique<int>();
-  P5.reset(((((new int)))));
+  P5.reset(((((new int())))));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_unique instead [modernize-make-unique]
   // CHECK-FIXES: P5 = std::make_unique<int>();
 
   {
     // No std.
     using namespace std;
     unique_ptr<int> Q = unique_ptr<int>(new int());
     // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use std::make_unique instead
     // CHECK-FIXES: unique_ptr<int> Q = std::make_unique<int>();
 
     Q = unique_ptr<int>(new int());
     // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use std::make_unique instead
     // CHECK-FIXES: Q = std::make_unique<int>();
+
+    Q = unique_ptr<int>(new int);
   }
 
   std::unique_ptr<int> R(new int());
 
   // Create the unique_ptr as a parameter to a function.
   int T = g(std::unique_ptr<int>(new int()));
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use std::make_unique instead
   // CHECK-FIXES: int T = g(std::make_unique<int>());
void initialization(int T, Base b) {
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning:
   // CHECK-FIXES: FFs = std::make_unique<Foo[]>(Num);
   int Num2 = 1;
   FFs.reset(new Foo[Num2]);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning:
   // CHECK-FIXES: FFs = std::make_unique<Foo[]>(Num2);
 
   std::unique_ptr<int[]> FI;
-  FI.reset(new int[5]()); // default initialization.
+  FI.reset(new int[5]()); // value initialization.
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
   // CHECK-FIXES: FI = std::make_unique<int[]>(5);
 
   // The check doesn't give warnings and fixes for cases where the original new
-  // expression doesn't do any initialization.
+  // expression does default initialization.
   FI.reset(new int[5]);
   FI.reset(new int[Num]);
   FI.reset(new int[Num2]);
 }
 
 void aliases() {
   typedef std::unique_ptr<int> IntPtr;
-  IntPtr Typedef = IntPtr(new int);
+  IntPtr Typedef = IntPtr(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use std::make_unique instead
   // CHECK-FIXES: IntPtr Typedef = std::make_unique<int>();
 
   // We use 'bool' instead of '_Bool'.
   typedef std::unique_ptr<bool> BoolPtr;
-  BoolPtr BoolType = BoolPtr(new bool);
+  BoolPtr BoolType = BoolPtr(new bool());
   // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use std::make_unique instead
   // CHECK-FIXES: BoolPtr BoolType = std::make_unique<bool>();
 
   // We use 'Base' instead of 'struct Base'.
   typedef std::unique_ptr<Base> BasePtr;
   BasePtr StructType = BasePtr(new Base);
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
 // CHECK-FIXES: BasePtr StructType = std::make_unique<Base>();
 
 #define PTR unique_ptr<int>
-  std::unique_ptr<int> Macro = std::PTR(new int);
+  std::unique_ptr<int> Macro = std::PTR(new int());
 // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: use std::make_unique instead
 // CHECK-FIXES: std::unique_ptr<int> Macro = std::make_unique<int>();
 #undef PTR
 
-  std::unique_ptr<int> Using = unique_ptr_<int>(new int);
+  std::unique_ptr<int> Using = unique_ptr_<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<int> Using = std::make_unique<int>();
 }
 
 void whitespaces() {
   // clang-format off
   auto Space = std::unique_ptr <int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use std::make_unique instead
   // CHECK-FIXES: auto Space = std::make_unique<int>();
 
   auto Spaces = std  ::    unique_ptr  <int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use std::make_unique instead
   // CHECK-FIXES: auto Spaces = std::make_unique<int>();
   // clang-format on
 }
 
 void nesting() {
   auto Nest = std::unique_ptr<std::unique_ptr<int>>(new std::unique_ptr<int>(new int));
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use std::make_unique instead
   // CHECK-FIXES: auto Nest = std::make_unique<std::unique_ptr<int>>(new int);
   Nest.reset(new std::unique_ptr<int>(new int));
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use std::make_unique instead
   // CHECK-FIXES: Nest = std::make_unique<std::unique_ptr<int>>(new int);
-  Nest->reset(new int);
+  Nest->reset(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use std::make_unique instead
   // CHECK-FIXES: *Nest = std::make_unique<int>();
 }
 
 void reset() {
   std::unique_ptr<int> P;
   P.reset();
   P.reset(nullptr);