[clang-tidy][modernize-make-unique] Checks c++14 flag before using std::make_unique

[ftingaud]

Summary

For a c++11 code, the clang-tidy rule "modernize-make-unique" should return immediately, as std::make_unique is not supported.

[aaron.ballman]

This fix is missing test coverage, can you add a C++11 and C++14 test to demonstrate the behavior differences?

clang-tidy/modernize/MakeSharedCheck.cpp

30 ↗

(On Diff #135912)

You can drop the clang:: from the parameter type.

clang-tidy/modernize/MakeUniqueCheck.cpp

39

Can drop it here as well.

[hokein]

Thanks for the patch!

The check provides MakeSmartPtrFunction option. Users can use it to customize their self-implemented make_unique function (instead of using the c++14 available std::make_unique) even in their C++11 code. I think we need to keep the backward compatibility.

I'd suggest only checking the C++14 flag when the suggested fix is "std::make_unique".

[alexfh]

clang-tidy/modernize/MakeSharedCheck.h

38 ↗

(On Diff #135912)

1. s/clang:://
2. it's not clear which "version" is this all about. I'd suggest isLanguageSupported/isLanguageVersionSupported or something else more specific than just "version".

[ftingaud]

Update with tests and cleaning as indicated by reviewers.

[ftingaud]

Remove two useless namespaces

[aaron.ballman]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

64

The formatting here is wrong.

[ftingaud]

Apply clang-format on changelist.

[alexfh]

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

5

I'd better use positive assertions with the original text. Otherwise the test can pass if the check does any replacements except for the ones you list in CHECK-FIXES-NOT.

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

8

As Haojian mentioned, this test still has to work in c++11.

[ftingaud]

Correct case of custom make_unique functions that should still work in c++11.

[aaron.ballman]

clang-tidy/modernize/MakeUniqueCheck.cpp

21

Why is this is a user-facing option?

If it needs to be a user-facing option, you also need to implement an override for storeOptions() as well.

25

What is this option? Why is this returning a string literal rather than something less error-prone like an enumeration?

[ftingaud]

clang-tidy/modernize/MakeUniqueCheck.cpp

21

As the rule was very customizable, I also made the c++ version customizable. But the option is only useful if a user has a custom make_unique that needs c++14 (uses std::make_unique internally) and multiple versions of C++ in their codeline. I agree this is a rather specific usecase. I can remove it and make the code simpler if it is your recommendation.

25

I made the MinimumLanguageVersion a string literal to make it customizable by the user. If we remove the customization, we can switch to an enum or a boolean.

[alexfh]

clang-tidy/modernize/MakeUniqueCheck.cpp

21

if a user has a custom make_unique that needs c++14

The only reason to have a custom make_unique that I know is to workaround the absence of std::make_unique in c++11. So this looks like a not very useful case to support.

[aaron.ballman]

clang-tidy/modernize/MakeUniqueCheck.cpp

21

Agreed, I'd drop it.

[• Quuxplusone]

clang-tidy/modernize/MakeUniqueCheck.cpp

21

From the peanut gallery: IIUC, yes, this is not a useful case to support.
If the user has a custom my::make_unique, then it is *by definition* usable in C++11. Now, it might still be implemented as a call to std::make_unique in C++14... but it will not *require* C++14.
The user's my::make_unique will likely require C++11 (not just C++03), but I personally don't think clang-tidy ought to cater to corner cases that involve C++03. I'd just assume that an instruction to "use my::make_unique" is appropriate for any translation unit that's being tidied with a custom make_unique.

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

11

IIUC above, you allow the user to pass in the name of a custom my::make_unique function. Could you add a test case for that?

[ftingaud]

Remove customizable c++ version that added no real value.

[ftingaud]

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

11

It is tested by modernize-make-unique-header.cpp and the patch doesn't change the feature.

[ftingaud]

Hi,

I think all requested modifications were done.

Regards,

Fred

[alexfh]

A couple of nits.

clang-tidy/modernize/MakeUniqueCheck.cpp

41–43

nit: Use ternary operator?

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

8

No need to define int main() in the test. It can also become confusing for the readers of the code. Please change this to void f() or something like that and remove the return statement.

Same below.

[ftingaud]

Clean code along code review recommendations.

[aaron.ballman]

LGTM!

[alexfh]

LG. Thank you!

[hokein]

LGTM.

I think all requested modifications were done.

Next time, please consider marking all the review comments done in phabricator, so that reviewers can see it obviously.

[alexfh]

Frederic, do you need someone to commit the patch for you?

[ftingaud]

Hi,
Yes please!

clang-tidy/modernize/MakeSmartPtrCheck.h

protected:
   using SmartPtrTypeMatcher = ast_matchers::internal::BindableMatcher<QualType>;
 
   /// Returns matcher that match with different smart pointer types.
   ///
   /// 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;

clang-tidy/modernize/MakeSmartPtrCheck.cpp

 
 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);
 }
 
+bool MakeSmartPtrCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus11;
+}
+
 void MakeSmartPtrCheck::registerPPCallbacks(CompilerInstance &Compiler) {
-  if (getLangOpts().CPlusPlus11) {
+  if (isLanguageVersionSupported(getLangOpts())) {
     Inserter.reset(new utils::IncludeInserter(
         Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
     Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
   }
 }
 
 void MakeSmartPtrCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
-  if (!getLangOpts().CPlusPlus11)
+  if (!isLanguageVersionSupported(getLangOpts()))
     return;
 
   // Calling make_smart_ptr from within a member function of a type with a
   // private or protected constructor would be ill-formed.
   auto CanCallCtor = unless(has(ignoringImpCasts(
       cxxConstructExpr(hasDeclaration(decl(unless(isPublic())))))));
 
   Finder->addMatcher(

clang-tidy/modernize/MakeUniqueCheck.h

 ///   std::make_unique<type>(args...)
 /// \endcode
 class MakeUniqueCheck : public MakeSmartPtrCheck {
 public:
   MakeUniqueCheck(StringRef Name, ClangTidyContext *Context);
 
 protected:
   SmartPtrTypeMatcher getSmartPointerTypeMatcher() const override;
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKE_UNIQUE_H

clang-tidy/modernize/MakeUniqueCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 MakeUniqueCheck::MakeUniqueCheck(StringRef Name,
                                  clang::tidy::ClangTidyContext *Context)
     : MakeSmartPtrCheck(Name, Context, "std::make_unique") {}
 
 MakeUniqueCheck::SmartPtrTypeMatcher
 MakeUniqueCheck::getSmartPointerTypeMatcher() const {
   return qualType(hasUnqualifiedDesugaredType(
       recordType(hasDeclaration(classTemplateSpecializationDecl(
           hasName("::std::unique_ptr"), templateArgumentCountIs(2),
           hasTemplateArgument(
               0, templateArgument(refersToType(qualType().bind(PointerType)))),
           hasTemplateArgument(
               1, templateArgument(refersToType(
                      qualType(hasDeclaration(classTemplateSpecializationDecl(
                          hasName("::std::default_delete"),
                          templateArgumentCountIs(1),
                          hasTemplateArgument(
                              0, templateArgument(refersToType(qualType(
                                     equalsBoundNode(PointerType))))))))))))))));
 }
 
+bool MakeUniqueCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus14;
+}
+
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

+// RUN: %check_clang_tidy %s modernize-make-unique %t -- -- -std=c++11 \
+// RUN:   -I%S/Inputs/modernize-smart-ptr
+
+#include "unique_ptr.h"
+// CHECK-FIXES-NOT: #include <memory>
+
+int main() {
+  auto my_ptr = std::unique_ptr<int>(new int(1));
+  // CHECK-FIXES-NOT: auto my_ptr = std::make_unique<int>(1);
+  return 0;
+}

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

+// RUN: %check_clang_tidy %s modernize-make-unique %t -- -- -std=c++14 \
+// RUN:   -I%S/Inputs/modernize-smart-ptr
+
+#include "unique_ptr.h"
+// CHECK-FIXES: #include <memory>
+
+int main() {
+  auto my_ptr = std::unique_ptr<int>(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use std::make_unique instead
+  // CHECK-FIXES: auto my_ptr = std::make_unique<int>(1);
+  return 0;
+}

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

 // RUN: %check_clang_tidy %s modernize-make-unique %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN:     [{key: modernize-make-unique.MakeSmartPtrFunction, \
 // RUN:       value: 'my::MakeUnique'}, \
 // RUN:      {key: modernize-make-unique.MakeSmartPtrFunctionHeader, \
 // RUN:       value: 'make_unique_util.h'} \
 // RUN:     ]}" \
-// RUN:   -- -std=c++11 -I%S/Inputs/modernize-smart-ptr
+// RUN:   -- -std=c++14 -I%S/Inputs/modernize-smart-ptr
 
 #include "unique_ptr.h"
 // CHECK-FIXES: #include "make_unique_util.h"
 
 void f() {
   std::unique_ptr<int> P1 = std::unique_ptr<int>(new int());
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use my::MakeUnique instead
   // CHECK-FIXES: std::unique_ptr<int> P1 = my::MakeUnique<int>();
 }

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
+// RUN:   -- -std=c++14  -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>

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

-// RUN: %check_clang_tidy %s modernize-make-unique %t -- -- -std=c++11 \
+// RUN: %check_clang_tidy %s modernize-make-unique %t -- -- -std=c++14 \
 // RUN:   -I%S/Inputs/modernize-smart-ptr
 
 #include "unique_ptr.h"
 #include "initializer_list.h"
 // CHECK-FIXES: #include <memory>
 
 struct Base {
   Base();