[clang-tidy] Do not show incorrect fix in modernize-make-unique

[ilya-biryukov]

Summary

The case when initialize_list hides behind an implicit case was not
handled before.

[ilya-biryukov]

BTW, for a common use-case we can do the same trick that's being done for aggregate init:

new X({1,2,3}, 123, {a});

into

make_unique<X>(X({1,2,3}, 123, {a}));

I can try fixing this, but would want to land this first to fix the issue at hand.

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

278

I'm wondering how to properly check that no fixes were produced here.
Do we have a common way to express this?
Would CHECK-FIXES-NOT: ... mentioning the line numbers somehow work?

[aaron.ballman]

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

301

We can now remove the IgnoreImplicit() call here, can't we?

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

278

I believe we typically use CHECK-FIXES: <the original code blurb> to ensure that the fix does not change any code.

[ilya-biryukov]

• Remove redundant check.
• Actually check the code stays the same in tests.

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

301

Right. Thanks for spotting that!

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

278

That worked. Thanks!

[aaron.ballman]

LGTM!

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

bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
   // Returns true if the given constructor expression has any braced-init-list
   // argument, e.g.
   //   Foo({1, 2}, 1) => true
   //   Foo(Bar{1, 2}) => true
   //   Foo(1) => false
   //   Foo{1} => false
   auto HasListIntializedArgument = [](const CXXConstructExpr *CE) {
     for (const auto *Arg : CE->arguments()) {
+      Arg = Arg->IgnoreImplicit();
+
       if (isa<CXXStdInitializerListExpr>(Arg) || isa<InitListExpr>(Arg))
         return true;
       // Check whether we implicitly construct a class from a
       // std::initializer_list.
-      if (const auto *ImplicitCE =
-              dyn_cast<CXXConstructExpr>(Arg->IgnoreImplicit())) {
+      if (const auto *ImplicitCE = dyn_cast<CXXConstructExpr>(Arg)) {
         if (ImplicitCE->isStdInitListInitialization())
           return true;
       }
       return false;
     }
     return false;
   };
   switch (New->getInitializationStyle()) {

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

void initialization(int T, Base b) {
   // Aggregate initialization.
   std::unique_ptr<APair> PAggr = std::unique_ptr<APair>(new APair{T, 1});
   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<APair> PAggr = std::make_unique<APair>(APair{T, 1});
   PAggr.reset(new APair{T, 1});
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use std::make_unique instead
   // CHECK-FIXES: std::make_unique<APair>(APair{T, 1});
 
+  // Check aggregate init with intermediate temporaries.
+  std::unique_ptr<APair> PAggrTemp = std::unique_ptr<APair>(new APair({T, 1}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: use std::make_unique instead
+  // CHECK-FIXES: std::unique_ptr<APair> PAggrTemp = std::unique_ptr<APair>(new APair({T, 1}));
+  PAggrTemp.reset(new APair({T, 1}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use std::make_unique instead
+  // CHECK-FIXES: PAggrTemp.reset(new APair({T, 1}));
+
   // Test different kinds of initialization of the pointee, when the unique_ptr
   // is initialized with braces.
 
   // Direct initialization with parenthesis.
   std::unique_ptr<DPair> PDir3 = std::unique_ptr<DPair>{new DPair(3, T)};
   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<DPair> PDir3 = std::make_unique<DPair>(3, T);