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

(On Diff #202432)

s/ediable/elidable/

s/presented/present in the AST/

304 ↗

(On Diff #202432)

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 ↗

(On Diff #202432)

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

457 ↗

(On Diff #202432)

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 ↗

(On Diff #202432)

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

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

1 ↗

(On Diff #202432)

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 ↗

(On Diff #202432)

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 ↗

(On Diff #202434)

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 ↗

(On Diff #202432)

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 ↗

(On Diff #202434)

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

[gribozavr]

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

306 ↗

(On Diff #202434)

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 ↗

(On Diff #202434)

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

[gribozavr]

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

306 ↗

(On Diff #202434)

modernize-use-auto is only a heuristic.

[Eugene.Zelenko]

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

306 ↗

(On Diff #202434)

But set of processed situations are very reasonable.

[gribozavr]

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

306 ↗

(On Diff #202434)

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 ↗

(On Diff #202434)

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 ↗

(On Diff #202434)

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

(On Diff #202434)

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/trunk/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 elidable move constructor, it is present in the AST for
+        // C++11/14, e.g. Foo(Bar{1, 2}), the move constructor is around the
+        // init-list 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/trunk/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
+// FIXME: Fix the test code in C++2a mode.
 
 #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>();