[clang-tidy] Ignore macros in make-unique check.

[hokein]

Summary

The check doesn't fully support smart-ptr usages inside macros, which
may cause incorrect fixes, or even crashes, ignore them for now.

[alexfh]

LG with one comment.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

114 ↗

(On Diff #109520)

Seems to be a bit too broad. Maybe just disable the fix in macros? If warnings without fixes will be a problem in some codebases, we could then introduce the IgnoreMacros option as in readability-redundant-declaration, modernize-use-using, modernize-use-bool-literals, and modernize-use-default-member-init.

[hokein]

Add IgnoreMacros option.

[hokein]

Add a missing test file.

[hokein]

Please take another look.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

114 ↗

(On Diff #109520)

The idea sounds good to me. Done.

If warnings without fixes will be a problem in some codebases, we could then introduce the IgnoreMacros option

Didn't know there is IgnoreMacros option, but we are missing the option document for these checks. Will add them afterwards.

[alexfh]

LG with a couple of nits.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

54 ↗

(On Diff #109691)

Let's go with getLocalOrGlobal to be able to set the option for all checks at once. Other checks having this option do this already.

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

14 ↗

(On Diff #109691)

Please add CHECK-FIXES to verify that the code remains intact (both macro definition and expansion).

[hokein]

Address review comments.

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

protected:
   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,
                   const CXXNewExpr *New);
 
   void 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

 
 } // 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,
+MakeSmartPtrCheck::MakeSmartPtrCheck(StringRef Name,
+                                     ClangTidyContext* Context,
                                      StringRef MakeSmartPtrFunctionName)
     : ClangTidyCheck(Name, Context),
       IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
           Options.getLocalOrGlobal("IncludeStyle", "llvm"))),
       MakeSmartPtrFunctionHeader(
           Options.get("MakeSmartPtrFunctionHeader", StdMemoryHeader)),
       MakeSmartPtrFunctionName(
-          Options.get("MakeSmartPtrFunction", MakeSmartPtrFunctionName)) {}
+          Options.get("MakeSmartPtrFunction", MakeSmartPtrFunctionName)),
+      IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
 
 void MakeSmartPtrCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle", IncludeStyle);
   Options.store(Opts, "MakeSmartPtrFunctionHeader", MakeSmartPtrFunctionHeader);
   Options.store(Opts, "MakeSmartPtrFunction", MakeSmartPtrFunctionName);
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 void MakeSmartPtrCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   if (getLangOpts().CPlusPlus11) {
     Inserter.reset(new utils::IncludeInserter(
         Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
     Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
   }
else if (Reset)
     checkReset(SM, Reset, New);
 }
 
 void MakeSmartPtrCheck::checkConstruct(SourceManager &SM,
                                        const CXXConstructExpr *Construct,
                                        const QualType *Type,
                                        const CXXNewExpr *New) {
   SourceLocation ConstructCallStart = Construct->getExprLoc();
+  bool InMacro = ConstructCallStart.isMacroID();
+
+  if (InMacro && IgnoreMacros) {
+    return;
+  }
 
   bool Invalid = false;
   StringRef ExprStr = Lexer::getSourceText(
       CharSourceRange::getCharRange(
           ConstructCallStart, Construct->getParenOrBraceRange().getBegin()),
       SM, getLangOpts(), &Invalid);
   if (Invalid)
     return;
 
   auto Diag = diag(ConstructCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 
+  // Disable the fix in macros.
+  if (InMacro) {
+    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(
void MakeSmartPtrCheck::checkReset(SourceManager &SM,
                                    const CXXNewExpr *New) {
   const auto *Expr = cast<MemberExpr>(Reset->getCallee());
   SourceLocation OperatorLoc = Expr->getOperatorLoc();
   SourceLocation ResetCallStart = Reset->getExprLoc();
   SourceLocation ExprStart = Expr->getLocStart();
   SourceLocation ExprEnd =
       Lexer::getLocForEndOfToken(Expr->getLocEnd(), 0, SM, getLangOpts());
 
+  bool InMacro = ExprStart.isMacroID();
+
+  if (InMacro && IgnoreMacros) {
+    return;
+  }
+
   auto Diag = diag(ResetCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 
+  // Disable the fix in macros.
+  if (InMacro) {
+    return;
+  }
+
   Diag << FixItHint::CreateReplacement(
       CharSourceRange::getCharRange(OperatorLoc, ExprEnd),
       (llvm::Twine(" = ") + MakeSmartPtrFunctionName + "<" +
        GetNewExprName(New, SM, getLangOpts()) + ">")
           .str());
 
   if (Expr->isArrow())
     Diag << FixItHint::CreateInsertion(ExprStart, "*");

clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-make-unique.rst

.. option:: MakeSmartPtrFunctionHeader
 
    A string specifying the corresponding header of make-unique-ptr function.
    Default is `memory`.
 
 .. option:: IncludeStyle
 
    A string specifying which include-style is used, `llvm` or `google`. Default
    is `llvm`.
+
+.. option:: IgnoreMacros
+
+   If set to non-zero, the check will not give warnings inside macros. Default
+   is `1`.

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

+// RUN: %check_clang_tidy %s modernize-make-unique %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-make-unique.IgnoreMacros, value: 0}]}" \
+// RUN:   -- -std=c++11  -I%S/Inputs/modernize-smart-ptr
+
+#include "unique_ptr.h"
+
+class Foo {};
+class Bar {};
+#define DEFINE(...) __VA_ARGS__
+// CHECK-FIXES: {{^}}#define DEFINE(...) __VA_ARGS__{{$}}
+template<typename T>
+void g2(std::unique_ptr<Foo> *t) {
+  DEFINE(
+  // CHECK-FIXES: {{^ *}}DEFINE({{$}}
+      auto p = std::unique_ptr<Foo>(new Foo);
+      // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use std::make_unique instead
+      // CHECK-FIXES: {{^ *}}auto p = std::unique_ptr<Foo>(new Foo);{{$}}
+      t->reset(new Foo);
+      // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use std::make_unique instead
+      // CHECK-FIXES: {{^ *}}t->reset(new Foo);{{$}}
+      );
+      // CHECK-FIXES: {{^ *}});{{$}}
+}
+void macro() {
+  std::unique_ptr<Foo> *t;
+  g2<Bar>(t);
+}
+#undef DEFINE

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

void reset() {
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use std::make_unique instead
   // CHECK-FIXES: P = std::make_unique<int>();
 
   auto Q = &P;
   Q->reset(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use std::make_unique instead
   // CHECK-FIXES: *Q = std::make_unique<int>();
 }
+
+#define DEFINE(...) __VA_ARGS__
+template<typename T>
+void g2(std::unique_ptr<Foo> *t) {
+  DEFINE(auto p = std::unique_ptr<Foo>(new Foo); t->reset(new Foo););
+}
+void macro() {
+  std::unique_ptr<Foo> *t;
+  g2<bar::Bar>(t);
+}
+#undef DEFINE