[clang-tidy] Fix make-unique check to work in C++17 mode.

[hokein]

Summary

Previously, we intended to omit the check fix to the case when constructor has
any braced-init-list argument. But the HasListInitializedArgument was not
correct to handle all cases (Foo(Bar{1, 2}) will return false in C++14
mode).

This patch fixes it, corrects the tests, and makes the check to run at C++17 mode.

[gribozavr]

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

302

s/ediable/elidable/

s/presented/present in the AST/

304

Isn't it the other way round -- the move constructor is around the init-list constructor?

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

1

"-std=c++14-or-later" (will also cover c++2a etc.)

457

Why do we print the warning if the fixit is not fixing anything?

[hokein]

Address comments

[hokein]

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

304

yeah, this is what I mean, updated the comment.

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

1

hmm, the test code has some compiler errors on c++2a mode, it is a different issue (I will address it in a follow-up), added a FIXME.

457

We may still want to inform users, these cases can still be converted to make_unique, but the check currently is not smart enough to give correct fixes.

[gribozavr]

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

311

So Foo(Bar{1, 2}) is not problematic, it is just that we can't tell it apart from Foo({1,2})?

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

457

Sorry I don't understand -- so what the file used to say, std::make_unique<J>(E{1, 2}, 1), is correct, and this patch introduces a regression? Or is some other syntax necessary?

Please add FIXMEs anyway.

[Eugene.Zelenko]

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

306

Return type is not mentioned explicitly, so auto should not be used.

[gribozavr]

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

306

An explicit type is not needed for readability here. The rule is to use auto when it improves readability, not when the type is not spelled in immediate vicinity.

[Eugene.Zelenko]

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

306

I think it's reasonable to follow modernize-use-auto.

[gribozavr]

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

306

modernize-use-auto is only a heuristic.

[Eugene.Zelenko]

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

306

But set of processed situations are very reasonable.

[gribozavr]

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

306

In abstract it might sound reasonable. In practice it is still a heuristic and not a law.

[Eugene.Zelenko]

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

306

I think it's reasonable to keep in memory that not everybody keeps functions/methods' return types in memory.

[gribozavr]

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

306

'auto' and the rules about 'auto' serve readability purposes. They are not for ensuring that types are always visible in the source code. Knowing the type is not the goal in itself. The types in the source code need to serve a purpose, like everything else we write. If you think that 'auto' is not reasonable here, I would ask you to explain why the code becomes less readable for an average developer familiar with Clang -- who knows what constructors are, and what constructor arguments are.

As an illustration, consider this code:

f(g());

We write code like this all the time and we are OK with not seeing the return type of g in the source code.

If we extract a variable to give it a descriptive name, we should be OK with 'auto' in:

auto descriptiveName = g();
f(x);

[hokein]

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

311

Yes, the Foo(Bar{1, 2}) is not problematic, we should ignore this case, as discussed, I'll change it in a follow-up patch.

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

bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
   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)) {
-        if (ImplicitCE->isStdInitListInitialization())
+      if (const auto *CEArg = dyn_cast<CXXConstructExpr>(Arg)) {
+        // Strip the ediable move constructor, it is presented in C++11/14 mode.
+        // e.g. Foo(Bar{1, 2}), the Bar init-list constructor is wrapped around
+        // an elidable move constructor.
+        if (CEArg->isElidable()) {
+          if (const auto *TempExp = CEArg->getArg(0)) {
+            if (const auto *UnwrappedCE =
+                    dyn_cast<CXXConstructExpr>(TempExp->IgnoreImplicit()))
+              CEArg = UnwrappedCE;
+          }
+        }
+        if (CEArg->isStdInitListInitialization())
           return true;
       }
-      return false;
     }
     return false;
   };
   switch (New->getInitializationStyle()) {
   case CXXNewExpr::NoInit: {
     if (ArraySizeExpr.empty()) {
       Diag << FixItHint::CreateRemoval(SourceRange(NewStart, NewEnd));
     } else {

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

-// RUN: %check_clang_tidy -std=c++14 %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy -std=c++14,c++17 %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr
 
 #include "unique_ptr.h"
 #include "initializer_list.h"
 // CHECK-FIXES: #include <memory>
 
 struct Base {
   Base();
   Base(int, int);
void initialization(int T, Base b) {
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<J> PJ1 = std::unique_ptr<J>(new J({1, 2}, 1));
   PJ1.reset(new J({1, 2}, 1));
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
   // CHECK-FIXES: PJ1.reset(new J({1, 2}, 1));
 
   std::unique_ptr<J> PJ2 = std::unique_ptr<J>(new J(E{1, 2}, 1));
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<J> PJ2 = std::make_unique<J>(E{1, 2}, 1);
+  // CHECK-FIXES: std::unique_ptr<J> PJ2 = std::unique_ptr<J>(new J(E{1, 2}, 1));
   PJ2.reset(new J(E{1, 2}, 1));
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
-  // CHECK-FIXES: PJ2 = std::make_unique<J>(E{1, 2}, 1);
+  // CHECK-FIXES: PJ2.reset(new J(E{1, 2}, 1));
 
   std::unique_ptr<J> PJ3 = std::unique_ptr<J>(new J{ {1, 2}, 1 });
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<J> PJ3 = std::unique_ptr<J>(new J{ {1, 2}, 1 });
   PJ3.reset(new J{ {1, 2}, 1 });
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
   // CHECK-FIXES:  PJ3.reset(new J{ {1, 2}, 1 });
 
   std::unique_ptr<J> PJ4 = std::unique_ptr<J>(new J{E{1, 2}, 1});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
-  // CHECK-FIXES: std::unique_ptr<J> PJ4 = std::make_unique<J>(E{1, 2}, 1);
+  // CHECK-FIXES: std::unique_ptr<J> PJ4 = std::unique_ptr<J>(new J{E{1, 2}, 1});
   PJ4.reset(new J{E{1, 2}, 1});
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
-  // CHECK-FIXES:  PJ4 = std::make_unique<J>(E{1, 2}, 1);
+  // CHECK-FIXES: PJ4.reset(new J{E{1, 2}, 1});
 
   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>();