[clang-tidy] Fix handling of parens around new expressions in make_<smartptr> checks.

[alexfh]

Summary

Extra parentheses around a new expression result in incorrect code
after applying fixes.

[hokein]

Thanks for fixing it. It looks much simpler than I thought originally.

Looks good, just a few nits.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

246 ↗

(On Diff #168671)

nit: I'd use auto to avoid this long type name, the type is clear to me, it is a lambda function.

250 ↗

(On Diff #168671)

nit: we can easily avoid the recursive call by using while here, I think.

[alexfh]

• Remove recursion, use auto.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

246 ↗

(On Diff #168671)

I first tried it with an auto, but the recursive call below didn't compile. Works fine without recursion though.

250 ↗

(On Diff #168671)

Turned out that iterative approach here doesn't complicate code as much as I thought it would.

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

protected:
   /// Requires to bind pointer type (qualType) with PointerType string declared
   /// in this class.
   virtual SmartPtrTypeMatcher getSmartPointerTypeMatcher() const = 0;
 
   /// Returns whether the C++ version is compatible with current check.
   virtual bool isLanguageVersionSupported(const LangOptions &LangOpts) const;
 
   static const char PointerType[];
-  static const char ConstructorCall[];
-  static const char ResetCall[];
-  static const char NewExpression[];
 
 private:
   std::unique_ptr<utils::IncludeInserter> Inserter;
   const utils::IncludeSorter::IncludeStyle IncludeStyle;
   const std::string MakeSmartPtrFunctionHeader;
   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,
+  void checkConstruct(SourceManager &SM, ASTContext *Ctx,
+                      const CXXConstructExpr *Construct, const QualType *Type,
                       const CXXNewExpr *New);
+  void checkReset(SourceManager &SM, ASTContext *Ctx,
+                  const CXXMemberCallExpr *Member, const CXXNewExpr *New);
 
   /// Returns true when the fixes for replacing CXXNewExpr are generated.
   bool replaceNew(DiagnosticBuilder &Diag, const CXXNewExpr *New,
-                  SourceManager &SM);
+                  SourceManager &SM, ASTContext *Ctx);
   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

 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 namespace {
 
 constexpr char StdMemoryHeader[] = "memory";
+constexpr char ConstructorCall[] = "constructorCall";
+constexpr char ResetCall[] = "resetCall";
+constexpr char NewExpression[] = "newExpression";
 
 std::string GetNewExprName(const CXXNewExpr *NewExpr,
                            const SourceManager &SM,
                            const LangOptions &Lang) {
   StringRef WrittenName = Lexer::getSourceText(
       CharSourceRange::getTokenRange(
           NewExpr->getAllocatedTypeSourceInfo()->getTypeLoc().getSourceRange()),
       SM, Lang);
   if (NewExpr->isArray()) {
-    return WrittenName.str() + "[]";
+    return (WrittenName + "[]").str();
   }
   return WrittenName.str();
 }
 
 } // namespace
 
 const char MakeSmartPtrCheck::PointerType[] = "pointerType";
-const char MakeSmartPtrCheck::ConstructorCall[] = "constructorCall";
-const char MakeSmartPtrCheck::ResetCall[] = "resetCall";
-const char MakeSmartPtrCheck::NewExpression[] = "newExpression";
 
 MakeSmartPtrCheck::MakeSmartPtrCheck(StringRef Name,
                                      ClangTidyContext* Context,
                                      StringRef MakeSmartPtrFunctionName)
     : ClangTidyCheck(Name, Context),
       IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
           Options.getLocalOrGlobal("IncludeStyle", "llvm"))),
       MakeSmartPtrFunctionHeader(

 
 bool MakeSmartPtrCheck::isLanguageVersionSupported(
     const LangOptions &LangOpts) const {
   return LangOpts.CPlusPlus11;
 }
 
 void MakeSmartPtrCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   if (isLanguageVersionSupported(getLangOpts())) {
-    Inserter.reset(new utils::IncludeInserter(
-        Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
+    Inserter = llvm::make_unique<utils::IncludeInserter>(
+        Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle);
     Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
   }
 }
 
 void MakeSmartPtrCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
   if (!isLanguageVersionSupported(getLangOpts()))
     return;
 
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);
 
   if (New->getNumPlacementArgs() != 0)
     return;
 
   if (Construct)
-    checkConstruct(SM, Construct, Type, New);
+    checkConstruct(SM, Result.Context, Construct, Type, New);
   else if (Reset)
-    checkReset(SM, Reset, New);
+    checkReset(SM, Result.Context, Reset, New);
 }
 
-void MakeSmartPtrCheck::checkConstruct(SourceManager &SM,
+void MakeSmartPtrCheck::checkConstruct(SourceManager &SM, ASTContext *Ctx,
                                        const CXXConstructExpr *Construct,
                                        const QualType *Type,
                                        const CXXNewExpr *New) {
   SourceLocation ConstructCallStart = Construct->getExprLoc();
   bool InMacro = ConstructCallStart.isMacroID();
 
   if (InMacro && IgnoreMacros) {
     return;
void MakeSmartPtrCheck::checkConstruct(SourceManager &SM, ASTContext *Ctx,
   auto Diag = diag(ConstructCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 
   // Disable the fix in macros.
   if (InMacro) {
     return;
   }
 
-  if (!replaceNew(Diag, New, SM)) {
+  if (!replaceNew(Diag, New, SM, Ctx)) {
     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)
Diag << FixItHint::CreateReplacement(
         CharSourceRange::getCharRange(BraceRange.getEnd(),
                                       BraceRange.getEnd().getLocWithOffset(1)),
         ")");
   }
 
   insertHeader(Diag, SM.getFileID(ConstructCallStart));
 }
 
-void MakeSmartPtrCheck::checkReset(SourceManager &SM,
+void MakeSmartPtrCheck::checkReset(SourceManager &SM, ASTContext *Ctx,
                                    const CXXMemberCallExpr *Reset,
                                    const CXXNewExpr *New) {
   const auto *Expr = cast<MemberExpr>(Reset->getCallee());
   SourceLocation OperatorLoc = Expr->getOperatorLoc();
   SourceLocation ResetCallStart = Reset->getExprLoc();
   SourceLocation ExprStart = Expr->getBeginLoc();
   SourceLocation ExprEnd =
       Lexer::getLocForEndOfToken(Expr->getEndLoc(), 0, SM, getLangOpts());
void MakeSmartPtrCheck::checkReset(SourceManager &SM, ASTContext *Ctx,
   auto Diag = diag(ResetCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 
   // Disable the fix in macros.
   if (InMacro) {
     return;
   }
 
-  if (!replaceNew(Diag, New, SM)) {
+  if (!replaceNew(Diag, New, SM, Ctx)) {
     return;
   }
 
   Diag << FixItHint::CreateReplacement(
       CharSourceRange::getCharRange(OperatorLoc, ExprEnd),
       (llvm::Twine(" = ") + MakeSmartPtrFunctionName + "<" +
        GetNewExprName(New, SM, getLangOpts()) + ">")
           .str());
 
   if (Expr->isArrow())
     Diag << FixItHint::CreateInsertion(ExprStart, "*");
 
   insertHeader(Diag, SM.getFileID(OperatorLoc));
 }
 
 bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
-                                   const CXXNewExpr *New,
-                                   SourceManager& SM) {
-  SourceLocation NewStart = New->getSourceRange().getBegin();
-  SourceLocation NewEnd = New->getSourceRange().getEnd();
+                                   const CXXNewExpr *New, SourceManager &SM,
+                                   ASTContext *Ctx) {
+  auto SkipParensParents = [&](const Expr *E) {
+    for (const Expr *OldE = nullptr; E != OldE;) {
+      OldE = E;
+      for (const auto &Node : Ctx->getParents(*E)) {
+        if (const Expr *Parent = Node.get<ParenExpr>()) {
+          E = Parent;
+          break;
+        }
+      }
+    }
+    return E;
+  };
+
+  SourceRange NewRange = SkipParensParents(New)->getSourceRange();
+  SourceLocation NewStart = NewRange.getBegin();
+  SourceLocation NewEnd = NewRange.getEnd();
 
   // Skip when the source location of the new expression is invalid.
   if (NewStart.isInvalid() || NewEnd.isInvalid())
     return false;
 
   std::string ArraySizeExpr;
   if (const auto* ArraySize = New->getArraySize()) {
     ArraySizeExpr = Lexer::getSourceText(CharSourceRange::getTokenRange(

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

void basic() {
   // 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));
+  // 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)));
+  // 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
     // CHECK-FIXES: shared_ptr<int> Q = std::make_shared<int>();
 
     Q = shared_ptr<int>(new int());

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

void basic() {
   // 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));
+  // 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));
+  // 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))));
+  // 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)))));
+  // 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());