[PATCH] D50852: [clang-tidy] abseil-auto-make-unique

[Jonas Toth via Phabricator via cfe-commits]

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