[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

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

250

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

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

250

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

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-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) {
+  std::function<const Expr *(const Expr *)> SkipParensParents =
+      [&](const Expr *E) {
+        for (const auto &Node : Ctx->getParents(*E)) {
+          if (const Expr *Parent = Node.get<ParenExpr>())
+            return SkipParensParents(Parent);
+        }
+        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(

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());

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());