JonasToth added inline comments.
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:21
+void AutoMakeUniqueCheck::registerMatchers(MatchFinder* finder) {
+ if (!getLangOpts().CPlusPlus) return;
+
----------------
Please clang-format, `return` on next line.
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:23
+
+ using clang::ast_matchers::isTemplateInstantiation;
+ auto is_instantiation = decl(anyOf(cxxRecordDecl(isTemplateInstantiation()),
----------------
`clang::ast_matchers` is used already at line 14. No need to add this line.
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:27
+ functionDecl(isTemplateInstantiation())));
+ // There should be no additional expressions inbetween.
+ // E.g. this statement contains implicitCastExpr which makes it not eligible:
----------------
This sentence misses something. inbetween what?
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:34
+ has(ignoringParenImpCasts(callExpr(callee(functionDecl(
+ hasAnyName("absl::MakeUnique", "absl::make_unique",
+ "gtl::MakeUnique", "std::make_unique"),
----------------
The same rules apply for `make_shared`.
`make_pair` would be interesting as well, are there are standard `make_`
templates?
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:51
+ var->getType()->getAsCXXRecordDecl());
+ if (!unique) return nullptr;
+ QualType type = unique->getTemplateArgs().get(0).getAsType();
----------------
Formatting and Naming conventions.
I think the following is more readable:
```
if (const auto* Unique = dyn_cast<ClassTemplateSpecializationDecl>(var...)) {
// stuff with var
}
return nullptr;
```
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:57
+const Type* GetMakeUniqueType(const FunctionDecl* make_unique_decl) {
+ const auto& template_arg =
+
make_unique_decl->getTemplateSpecializationInfo()->TemplateArguments->get(
----------------
Naming conventions.
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:64
+void AutoMakeUniqueCheck::check(
+ const ast_matchers::MatchFinder::MatchResult& result) {
+ const auto* var_decl = result.Nodes.getNodeAs<VarDecl>("var_decl");
----------------
You don't need the `ast_matchers` namespace here.
Please follow the naming convention inside the function.
================
Comment at: clang-tidy/abseil/AutoMakeUniqueCheck.cpp:69
+ if (var_decl->isOutOfLine()) {
+ // "auto Struct::field = make_unique<...>();" doesn't work in GCC.
+ return;
----------------
What exactly is the issue here in GCC? Please make this comment more
explanatory and maybe add a `FIXME` to work around the issue (depending on the
nature of the issue)
================
Comment at: docs/ReleaseNotes.rst:63
+
+ FIXME: add release notes.
+
----------------
Please add release notes.
================
Comment at: docs/clang-tidy/checks/abseil-auto-make-unique.rst:11
+ std::unique_ptr<Foo> x = MakeUnique<Foo>(...);
+
+with
----------------
Please add documentation that the 'Abstract Factory' use case is resolved
correctly.
================
Comment at: test/clang-tidy/abseil-auto-make-unique.cpp:34
+void Primitive() {
+ std::unique_ptr<int> x = absl::make_unique<int>();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating
the type name
----------------
How does the code look with multiple variable declarations?
Please add a test for
```
std::unique_ptr<int> x = absl::make_unique<int>(), y =
absl::make_unique<int>(), z;
```
================
Comment at: test/clang-tidy/abseil-auto-make-unique.cpp:73
+ // Different type. No change.
+ std::unique_ptr<Base> z = make_unique<Derived>();
+ std::unique_ptr<Base> z2(make_unique<Derived>());
----------------
lets consider a 3 level class hierarchy.
```
struct A { virtual void Something(); };
struct B : A { void Something() override; };
struct C : B { void Something() override; };
std::unique_ptr<B> b_ptr = make_unique<B>(), c_ptr = make_unique<C>();
std::unique_ptr<B> c_ptr2 = make_unique<C>(), b_ptr2 = make_unique<B>();
```
What is the behaviour? I expect that these places break when transformed. To
avoid you can check the `VarDecl` `isSingleDecl()` (or similar, i forgot the
exact name) and only emit a fixit if it is.
Doing type transformations for the multi-definitions is tricky.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50852
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
