[clang-tidy] Don't generate fixes for initializer_list constructor in make_unique check.

[hokein]

Summary

The current fix will break the compilation -- because braced list is not
deducible in std::make_unique (with the use of forwarding) without
specifying the type explicitly.

We could support it in the future.

[alexfh]

LG

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

253 ↗

(On Diff #111320)

I'd leave CHECK-FIXES with the original text to ensure no unwanted replacements are performed.

[hokein]

Add CHECK-FIXES, and fix an uncaught issue.

[hokein]

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

253 ↗

(On Diff #111320)

Indeed, this is a good suggestion, and it discovered an issue of this patch (there are unexpected replacements generated for these cases).

I have updated the patch which changes a bit more code, please take a look again.

[alexfh]

Still LG with one comment.

clang-tidy/modernize/MakeSmartPtrCheck.h

60 ↗

(On Diff #111481)

I'd suggest using a less ambiguous phrase: instead of "Returns whether X." I write "Returns \c true when X".

[hokein]

Address review comments.

clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h

private:
   const std::string MakeSmartPtrFunctionName;
   const bool IgnoreMacros;
 
   void checkConstruct(SourceManager &SM, const CXXConstructExpr *Construct,
                       const QualType *Type, const CXXNewExpr *New);
   void checkReset(SourceManager &SM, const CXXMemberCallExpr *Member,
                   const CXXNewExpr *New);
 
-  void replaceNew(DiagnosticBuilder &Diag, const CXXNewExpr *New,
+  /// Returns true when the fixes for replacing CXXNewExpr are generated.
+  bool replaceNew(DiagnosticBuilder &Diag, const CXXNewExpr *New,
                   SourceManager &SM);
   void insertHeader(DiagnosticBuilder &Diag, FileID FD);
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKE_SMART_PTR_H

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

void MakeSmartPtrCheck::checkConstruct(SourceManager &SM,
   auto Diag = diag(ConstructCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 
   // Disable the fix in macros.
   if (InMacro) {
     return;
   }
 
+  if (!replaceNew(Diag, New, SM)) {
+    return;
+  }
+
   // Find the location of the template's left angle.
   size_t LAngle = ExprStr.find("<");
   SourceLocation ConstructCallEnd;
   if (LAngle == StringRef::npos) {
     // If the template argument is missing (because it is part of the alias)
     // we have to add it back.
     ConstructCallEnd = ConstructCallStart.getLocWithOffset(ExprStr.size());
     Diag << FixItHint::CreateInsertion(
Diag << FixItHint::CreateReplacement(
             BraceRange.getBegin(), BraceRange.getBegin().getLocWithOffset(1)),
         "(");
     Diag << FixItHint::CreateReplacement(
         CharSourceRange::getCharRange(BraceRange.getEnd(),
                                       BraceRange.getEnd().getLocWithOffset(1)),
         ")");
   }
 
-  replaceNew(Diag, New, SM);
   insertHeader(Diag, SM.getFileID(ConstructCallStart));
 }
 
 void MakeSmartPtrCheck::checkReset(SourceManager &SM,
                                    const CXXMemberCallExpr *Reset,
                                    const CXXNewExpr *New) {
   const auto *Expr = cast<MemberExpr>(Reset->getCallee());
   SourceLocation OperatorLoc = Expr->getOperatorLoc();
void MakeSmartPtrCheck::checkReset(SourceManager &SM,
   auto Diag = diag(ResetCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 
   // Disable the fix in macros.
   if (InMacro) {
     return;
   }
 
+  if (!replaceNew(Diag, New, SM)) {
+    return;
+  }
+
   Diag << FixItHint::CreateReplacement(
       CharSourceRange::getCharRange(OperatorLoc, ExprEnd),
       (llvm::Twine(" = ") + MakeSmartPtrFunctionName + "<" +
        GetNewExprName(New, SM, getLangOpts()) + ">")
           .str());
 
   if (Expr->isArrow())
     Diag << FixItHint::CreateInsertion(ExprStart, "*");
 
-  replaceNew(Diag, New, SM);
   insertHeader(Diag, SM.getFileID(OperatorLoc));
 }
 
-void MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
+bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
                                    const CXXNewExpr *New,
                                    SourceManager& SM) {
   SourceLocation NewStart = New->getSourceRange().getBegin();
   SourceLocation NewEnd = New->getSourceRange().getEnd();
 
   std::string ArraySizeExpr;
   if (const auto* ArraySize = New->getArraySize()) {
     ArraySizeExpr = Lexer::getSourceText(CharSourceRange::getTokenRange(
if (ArraySizeExpr.empty()) {
       // New array expression without written initializer:
       //   smart_ptr<Foo[]>(new Foo[5]);
       Diag << FixItHint::CreateReplacement(SourceRange(NewStart, NewEnd),
                                            ArraySizeExpr);
     }
     break;
   }
   case CXXNewExpr::CallInit: {
+    // FIXME: Add fixes for constructors with initializer-list parameters.
+    // Unlike ordinal cases, braced list can not be deduced in
+    // std::make_smart_ptr, we need to specify the type explicitly in the fixes:
+    //   struct S { S(std::initializer_list<int>, int); };
+    //   smart_ptr<S>(new S({1, 2, 3}, 1));  // C++98 call-style initialization
+    //   smart_ptr<S>(new S({}, 1));
+    // The above samples have to be replaced with:
+    //   std::make_smart_ptr<S>(std::initializer_list<int>({1, 2, 3}), 1);
+    //   std::make_smart_ptr<S>(std::initializer_list<int>({}), 1);
+    if (const auto *CE = New->getConstructExpr()) {
+      for (const auto *Arg : CE->arguments()) {
+        if (llvm::isa<CXXStdInitializerListExpr>(Arg)) {
+          return false;
+        }
+      }
+    }
     if (ArraySizeExpr.empty()) {
       SourceRange InitRange = New->getDirectInitRange();
       Diag << FixItHint::CreateRemoval(
           SourceRange(NewStart, InitRange.getBegin()));
       Diag << FixItHint::CreateRemoval(SourceRange(InitRange.getEnd(), NewEnd));
     }
     else {
       // New array expression with default/value initialization:
       //   smart_ptr<Foo[]>(new int[5]());
       //   smart_ptr<Foo[]>(new Foo[5]());
       Diag << FixItHint::CreateReplacement(SourceRange(NewStart, NewEnd),
                                            ArraySizeExpr);
     }
     break;
   }
   case CXXNewExpr::ListInit: {
     // Range of the substring that we do not want to remove.
     SourceRange InitRange;
     if (const auto *NewConstruct = New->getConstructExpr()) {
       if (NewConstruct->isStdInitListInitialization()) {
-        // Direct Initialization with the initializer-list constructor.
-        //   struct S { S(std::initializer_list<T>); };
-        //   smart_ptr<S>(new S{1, 2, 3});
-        //   smart_ptr<S>(new S{}); // use initializer-list consturctor
-        // The brace has to be kept, so this has to be replaced with:
-        //   std::make_smart_ptr<S>({1, 2, 3});
-        //   std::make_smart_ptr<S>({});
-        unsigned NumArgs = NewConstruct->getNumArgs();
-        if (NumArgs == 0) {
-          return;
-        }
-        InitRange = SourceRange(NewConstruct->getArg(0)->getLocStart(),
-                                NewConstruct->getArg(NumArgs - 1)->getLocEnd());
+        // FIXME: Add fixes for direct initialization with the initializer-list
+        // constructor. Similar to the above CallInit case, the type has to be
+        // specified explicitly in the fixes.
+        //   struct S { S(std::initializer_list<int>); };
+        //   smart_ptr<S>(new S{1, 2, 3});  // C++11 direct list-initialization
+        //   smart_ptr<S>(new S{});  // use initializer-list consturctor
+        // The above cases have to be replaced with:
+        //   std::make_smart_ptr<S>(std::initializer_list<int>({1, 2, 3}));
+        //   std::make_smart_ptr<S>(std::initializer_list<int>({}));
+        return false;
       } else {
         // Direct initialization with ordinary constructors.
         //   struct S { S(int x); S(); };
         //   smart_ptr<S>(new S{5});
         //   smart_ptr<S>(new S{}); // use default constructor
         // The arguments in the initialization list are going to be forwarded to
         // the constructor, so this has to be replaced with:
         //   std::make_smart_ptr<S>(5);
case CXXNewExpr::ListInit: {
     }
     Diag << FixItHint::CreateRemoval(
         CharSourceRange::getCharRange(NewStart, InitRange.getBegin()));
     Diag << FixItHint::CreateRemoval(
         SourceRange(InitRange.getEnd().getLocWithOffset(1), NewEnd));
     break;
   }
   }
+  return true;
 }
 
 void MakeSmartPtrCheck::insertHeader(DiagnosticBuilder &Diag, FileID FD) {
   if (MakeSmartPtrFunctionHeader.empty()) {
     return;
   }
   if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
           FD, MakeSmartPtrFunctionHeader,
           /*IsAngled=*/MakeSmartPtrFunctionHeader == StdMemoryHeader)) {
     Diag << *IncludeFixit;
   }
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

void initialization(int T, Base b) {
   std::unique_ptr<Empty> PEmpty = std::unique_ptr<Empty>(new Empty{});
   // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<Empty> PEmpty = std::make_unique<Empty>(Empty{});
 
   // Initialization with default constructor.
   std::unique_ptr<E> PE1 = std::unique_ptr<E>(new E{});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<E> PE1 = std::make_unique<E>();
+  PE1.reset(new E{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PE1 = std::make_unique<E>();
+
+  //============================================================================
+  //  NOTE: For initlializer-list constructors, the check only gives warnings,
+  //  and no fixes are generated.
+  //============================================================================
 
   // Initialization with the initializer-list constructor.
   std::unique_ptr<E> PE2 = std::unique_ptr<E>(new E{1, 2});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<E> PE2 = std::make_unique<E>({1, 2});
+  // CHECK-FIXES: std::unique_ptr<E> PE2 = std::unique_ptr<E>(new E{1, 2});
+  PE2.reset(new E{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PE2.reset(new E{1, 2});
 
   // Initialization with default constructor.
   std::unique_ptr<F> PF1 = std::unique_ptr<F>(new F());
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<F> PF1 = std::make_unique<F>();
+  PF1.reset(new F());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PF1 = std::make_unique<F>();
 
   // Initialization with default constructor.
   std::unique_ptr<F> PF2 = std::unique_ptr<F>(new F{});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<F> PF2 = std::make_unique<F>();
+  PF2.reset(new F());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PF2 = std::make_unique<F>();
 
   // Initialization with the initializer-list constructor.
   std::unique_ptr<F> PF3 = std::unique_ptr<F>(new F{1});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<F> PF3 = std::make_unique<F>({1});
+  // CHECK-FIXES: std::unique_ptr<F> PF3 = std::unique_ptr<F>(new F{1});
+  PF3.reset(new F{1});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PF3.reset(new F{1});
 
   // Initialization with the initializer-list constructor.
   std::unique_ptr<F> PF4 = std::unique_ptr<F>(new F{1, 2});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<F> PF4 = std::make_unique<F>({1, 2});
+  // CHECK-FIXES: std::unique_ptr<F> PF4 = std::unique_ptr<F>(new F{1, 2});
 
   // Initialization with the initializer-list constructor.
   std::unique_ptr<F> PF5 = std::unique_ptr<F>(new F({1, 2}));
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<F> PF5 = std::make_unique<F>({1, 2});
+  // CHECK-FIXES: std::unique_ptr<F> PF5 = std::unique_ptr<F>(new F({1, 2}));
 
   // Initialization with the initializer-list constructor as the default
   // constructor is not present.
   std::unique_ptr<G> PG1 = std::unique_ptr<G>(new G{});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<G> PG1 = std::make_unique<G>({});
+  // CHECK-FIXES: std::unique_ptr<G> PG1 = std::unique_ptr<G>(new G{});
+  PG1.reset(new G{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PG1.reset(new G{});
 
   // Initialization with the initializer-list constructor.
   std::unique_ptr<G> PG2 = std::unique_ptr<G>(new G{1});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<G> PG2 = std::make_unique<G>({1});
+  // CHECK-FIXES: std::unique_ptr<G> PG2 = std::unique_ptr<G>(new G{1});
 
   // Initialization with the initializer-list constructor.
   std::unique_ptr<G> PG3 = std::unique_ptr<G>(new G{1, 2});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<G> PG3 = std::make_unique<G>({1, 2});
+  // CHECK-FIXES: std::unique_ptr<G> PG3 = std::unique_ptr<G>(new G{1, 2});
 
   std::unique_ptr<Foo> FF = std::unique_ptr<Foo>(new Foo());
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning:
   // CHECK-FIXES: std::unique_ptr<Foo> FF = std::make_unique<Foo>();
   FF.reset(new Foo());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
   // CHECK-FIXES: FF = std::make_unique<Foo>();