[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik closed this revision.
shafik added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

Closing as it look like this is now `modernize-make-unique`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

In D55044#1589496 , @alexfh wrote:

> Seems good now. Haojian, do you have any concerns?


sorry for the looong delay, I totally miss this thread. looks good from my 
side, you'd need to rebase the patch to master.




Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:22
+The Abseil Style Guide _ discusses this issue in
+more detail.

I think we should expose the `MakeSmartPtrFunctionHeader` to the document, this 
option is used to configure the absl memory header.



Comment at: test/clang-tidy/abseil-make-unique.cpp:1
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+// RUN:   -I%S/Inputs/modernize-smart-ptr

nit: drop the `-std=c++11`, check_clang_tidy.py will add it by default.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Seems good now. Haojian, do you have any concerns?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang added a comment.

Pinging for visibility.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang updated this revision to Diff 195791.
axzhang added a comment.

Simplify tests and fix issues with Options.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-make-unique.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-make-unique.rst
  test/clang-tidy/abseil-make-unique.cpp

Index: test/clang-tidy/abseil-make-unique.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-make-unique.cpp
@@ -0,0 +1,67 @@
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+// RUN:   -I%S/Inputs/modernize-smart-ptr
+
+#include "unique_ptr.h"
+#include "initializer_list.h"
+// CHECK-FIXES: #include "absl/memory/memory.h"
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct B {
+  B(std::initializer_list);
+  B();
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+
+std::unique_ptr makeAndReturnPointer() {
+  return std::unique_ptr(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique(0);
+}
+
+void Positives() {
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique(2);
+
+  // Non-primitive paramter
+  std::unique_ptr P2 = std::unique_ptr(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P2 = absl::make_unique(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique(3, 4);
+}
+
+void Negatives() {
+  // Only warn if explicitly allocating a new object
+  std::unique_ptr R = std::unique_ptr(returnPointer());
+  R.reset(returnPointer());
+
+  // Only replace if the template type is same as new type
+  auto Pderived = std::unique_ptr(new Derived());
+
+  // Ignore initializer-list constructors
+  std::unique_ptr PInit = std::unique_ptr(new B{1, 2});
+  PInit.reset(new B{1, 2});
+}
Index: docs/clang-tidy/checks/modernize-make-unique.rst
===
--- docs/clang-tidy/checks/modernize-make-unique.rst
+++ docs/clang-tidy/checks/modernize-make-unique.rst
@@ -48,3 +48,9 @@
 
If set to non-zero, the check will not give warnings inside macros. Default
is `1`.
+
+.. option:: IgnoreListInit
+   
+   If set to non-zero, the check will ignore list initializations of `new`
+   expressions. Default is `0`.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -13,6 +13,7 @@
abseil-duration-subtraction
abseil-duration-unnecessary-conversion
abseil-faster-strsplit-delimiter
+   abseil-make-unique (redirects to modernize-make-unique) 
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
Index: docs/clang-tidy/checks/abseil-make-unique.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-make-unique.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - abseil-make-unique
+.. meta::
+   :http-equiv=refresh: 5;URL=abseil-make-unique.html
+
+abseil-make-unique
+==
+
+This check finds the creation of ``std::unique_ptr`` objects by explicitly
+calling the constructor and a ``new`` expression, and replaces it with a call
+to ``absl::make_unique``, the Abseil implementation of ``std::make_unique`` 
+in C++11.
+
+.. code-block:: c++
+
+  auto ptr = std::unique_ptr(new int(1));
+
+  // becomes
+
+  auto ptr = absl::make_unique(1);
+
+The Abseil Style Guide _ discusses this issue in
+more detail.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,11 @@
   Finds and fixes cases where ``absl::Duration`` values are being converted to
   numeric types and back again.
 
+- New alias :doc:`abseil-make-unique
+  ` to :doc:`modernize-make-unique
+  `
+  added.
+
 - New :doc:`abseil-time-subtraction
   ` check.
 
@@ -104,6 +109,11 @@
   `CommentUserDefiniedLiterals`, `CommentStringLiterals`,
   `CommentCharacterLiterals` & 

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang marked 3 inline comments as done.
axzhang added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:69
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+  IgnoreListInit(Options.get("IgnoreListInit", false)) {}
 

hintonda wrote:
> hintonda wrote:
> > axzhang wrote:
> > > hintonda wrote:
> > > > You’re setting it false here.
> > > I thought that false was the default value if the options do not contain 
> > > "IgnoreListInit"?
> > Do you have a test that passes this option?  I didn’t see one.
> I could be wrong, but when you do an Options.get(), that looks at what was 
> passed on the commandline, and if it wasn't found, it sets the default 
> provided.
Fixed by changing the Options value to "1" instead of true in the 
AbseilTidyModule


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Don Hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: clang-tidy/abseil/AbseilTidyModule.cpp:77
+Opts["abseil-make-unique.MakeSmartPtrFunction"] = "absl::make_unique";
+Opts["abseil-make-unique.IgnoreListInit"] = true;
+return Options;

This is defined as `typedef std::map OptionMap;`, so 
you need to assign a string, not a literal `true` value.  hth...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Don Hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:69
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+  IgnoreListInit(Options.get("IgnoreListInit", false)) {}
 

hintonda wrote:
> axzhang wrote:
> > hintonda wrote:
> > > You’re setting it false here.
> > I thought that false was the default value if the options do not contain 
> > "IgnoreListInit"?
> Do you have a test that passes this option?  I didn’t see one.
I could be wrong, but when you do an Options.get(), that looks at what was 
passed on the commandline, and if it wasn't found, it sets the default provided.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Don Hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:69
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+  IgnoreListInit(Options.get("IgnoreListInit", false)) {}
 

axzhang wrote:
> hintonda wrote:
> > You’re setting it false here.
> I thought that false was the default value if the options do not contain 
> "IgnoreListInit"?
Do you have a test that passes this option?  I didn’t see one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang marked an inline comment as done.
axzhang added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:69
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+  IgnoreListInit(Options.get("IgnoreListInit", false)) {}
 

hintonda wrote:
> You’re setting it false here.
I thought that false was the default value if the options do not contain 
"IgnoreListInit"?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Don Hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:69
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+  IgnoreListInit(Options.get("IgnoreListInit", false)) {}
 

You’re setting it false here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang marked an inline comment as done.
axzhang added a comment.

I'm having some issues with the AbseilTidyModule options. I am storing 
IgnoreListInit as true for the abseil-make-unique check, but when I run the 
check it shows IgnoreListInit as being false. Any ideas why this is happening?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, looks better now.
I saw there are still a few undone comments, please mark them done when you 
address them.




Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:19
+
+per the Abseil tips and guidelines at https://abseil.io/tips/126.

Eugene.Zelenko wrote:
> Please use hyperlink like:
> 
> The `Abseil Style Guide `_ discusses this issue 
> in more detail.
This comment is undone.



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:8
+
+The abseil-make-unique check is an alias, please see
+`modernize-make-unique `_ for more information.

This is not really alias, `abseil-make-unique` is different than 
`modernize-make-unique`, please add more details to the document here.



Comment at: test/clang-tidy/abseil-make-unique.cpp:9
+template >
+class unique_ptr {
+public:

we have an implementation in Inputs/modernize-smart-ptr/unique_ptr.h, please 
use it.




Comment at: test/clang-tidy/abseil-make-unique.cpp:59
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead 
[abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);

since the check shares the underlying modernize-make-unique implementation, I 
think there is no need to repeat most test cases here (those are covered in 
`test/clang-tidy/modernize-make-unique.cpp`).

I'd suggest

- add a simple test to verify this check provides abseil-related fixes (abseil 
header, absl::make_unique);
- adding test cases for list initialization (make sure we ignore those in this 
check);



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang added a comment.

If no more changes need to be made, is this okay to be merged in?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang updated this revision to Diff 194435.
axzhang marked an inline comment as not done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-make-unique.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-make-unique.rst
  test/clang-tidy/abseil-make-unique.cpp

Index: test/clang-tidy/abseil-make-unique.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-make-unique.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+
+namespace std {
+
+template 
+class default_delete {};
+
+template >
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr ) = delete;
+  unique_ptr(unique_ptr &) {}
+  ~unique_ptr() {}
+  type *() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr =(unique_ptr &&) { return *this; }
+  template 
+  unique_ptr =(unique_ptr &&) { return *this; }
+
+private:
+  type *ptr;
+};
+
+}  // namespace std
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+void expectPointer(std::unique_ptr p);
+
+std::unique_ptr makeAndReturnPointer() {
+  return std::unique_ptr(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique(0);
+}
+
+void Positives() {
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique(2);
+
+  // Non-primitive paramter
+  std::unique_ptr P2 = std::unique_ptr(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P2 = absl::make_unique(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique(3, 4);
+
+  // No arguments to new expression
+  std::unique_ptr P3 = std::unique_ptr(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P3 = absl::make_unique();
+
+  P3.reset(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P3 = absl::make_unique();
+
+  // Nested parentheses
+  std::unique_ptr P4 = std::unique_ptr((new int(3)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P4 = absl::make_unique(3);
+
+  P4 = std::unique_ptr(((new int(4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(4);
+
+  P4.reset((new int(5)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(5);
+
+  // With auto
+  auto P5 = std::unique_ptr(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: auto P5 = absl::make_unique();
+
+  {
+// No std
+using namespace std;
+unique_ptr Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: unique_ptr Q = absl::make_unique();
+
+Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: Q = absl::make_unique();
+  }
+
+  // Create the unique_ptr as a parameter to a function
+  expectPointer(std::unique_ptr(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: expectPointer(absl::make_unique());
+}
+
+void Negatives() {
+  // Only warn if explicitly allocating a new object
+  std::unique_ptr R = std::unique_ptr(returnPointer());
+  R.reset(returnPointer());
+
+  // Only replace if the template type is same as new type
+  auto Pderived = std::unique_ptr(new Derived());
+}
Index: docs/clang-tidy/checks/modernize-make-unique.rst
===
--- 

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.

Hmm, i suppose this looks good to me now.
It would be good to early-exit, but i'm not sure how important that is.
Please see D52771  / 
`clang-tools-extra/trunk/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp`
on how to use the bool params to short-circuit the astmatchers.




Comment at: docs/ReleaseNotes.rst:114
+  ` now supports an
+  `IgnoreListInit` option.
+

... which does xyz


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang marked 2 inline comments as done and an inline comment as not done.
axzhang added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:94
  equalsBoundNode(PointerType),
  CanCallCtor)
   .bind(NewExpression)),

axzhang wrote:
> lebedev.ri wrote:
> > ```
> > CanCallCtor, anyOf(unless(IgnoreListInit), 
> > unless(hasInitializationStyle(CXXNewExpr::ListInit
> > ```
> I can't compile `unless(IgnoreListInit)`, but `unless(IgnoreListInit ? 
> unless(anything()) : anything())` does work. I'm not sure how idiomatic that 
> is though.
Actually, `unless(anything())` also does not compile. How should I make a 
matcher that won't trigger if `IgnoreListInit` is true?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang marked an inline comment as done.
axzhang added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:94
  equalsBoundNode(PointerType),
  CanCallCtor)
   .bind(NewExpression)),

lebedev.ri wrote:
> ```
> CanCallCtor, anyOf(unless(IgnoreListInit), 
> unless(hasInitializationStyle(CXXNewExpr::ListInit
> ```
I can't compile `unless(IgnoreListInit)`, but `unless(IgnoreListInit ? 
unless(anything()) : anything())` does work. I'm not sure how idiomatic that is 
though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

It is best to error-out early.
Could you please try this instead:




Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:39
 }
 
 } // namespace

```
AST_MATCHER_P(CXXNewExpr, hasInitializationStyle,
  CXXNewExpr::InitializationStyle, IS) {
  return Node.getInitializationStyle() == IS;
};
```



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:94
  equalsBoundNode(PointerType),
  CanCallCtor)
   .bind(NewExpression)),

```
CanCallCtor, anyOf(unless(IgnoreListInit), 
unless(hasInitializationStyle(CXXNewExpr::ListInit
```



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:104
   callee(cxxMethodDecl(hasName("reset"))),
   hasArgument(0, cxxNewExpr(CanCallCtor).bind(NewExpression)),
   unless(isInTemplateInstantiation()))

```
hasArgument(0, cxxNewExpr(CanCallCtor, anyOf(unless(IgnoreListInit), 
unless(hasInitializationStyle(CXXNewExpr::ListInit.bind(NewExpression)),
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang updated this revision to Diff 192394.
axzhang added a comment.

Add documentation about the added option for `modernize-make-unique`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-make-unique.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-make-unique.rst
  test/clang-tidy/abseil-make-unique.cpp

Index: test/clang-tidy/abseil-make-unique.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-make-unique.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+
+namespace std {
+
+template 
+class default_delete {};
+
+template >
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr ) = delete;
+  unique_ptr(unique_ptr &) {}
+  ~unique_ptr() {}
+  type *() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr =(unique_ptr &&) { return *this; }
+  template 
+  unique_ptr =(unique_ptr &&) { return *this; }
+
+private:
+  type *ptr;
+};
+
+}  // namespace std
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+void expectPointer(std::unique_ptr p);
+
+std::unique_ptr makeAndReturnPointer() {
+  return std::unique_ptr(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique(0);
+}
+
+void Positives() {
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique(2);
+
+  // Non-primitive paramter
+  std::unique_ptr P2 = std::unique_ptr(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P2 = absl::make_unique(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique(3, 4);
+
+  // No arguments to new expression
+  std::unique_ptr P3 = std::unique_ptr(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P3 = absl::make_unique();
+
+  P3.reset(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P3 = absl::make_unique();
+
+  // Nested parentheses
+  std::unique_ptr P4 = std::unique_ptr((new int(3)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P4 = absl::make_unique(3);
+
+  P4 = std::unique_ptr(((new int(4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(4);
+
+  P4.reset((new int(5)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(5);
+
+  // With auto
+  auto P5 = std::unique_ptr(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: auto P5 = absl::make_unique();
+
+  {
+// No std
+using namespace std;
+unique_ptr Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: unique_ptr Q = absl::make_unique();
+
+Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: Q = absl::make_unique();
+  }
+
+  // Create the unique_ptr as a parameter to a function
+  expectPointer(std::unique_ptr(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: expectPointer(absl::make_unique());
+}
+
+void Negatives() {
+  // Only warn if explicitly allocating a new object
+  std::unique_ptr R = std::unique_ptr(returnPointer());
+  R.reset(returnPointer());
+
+  // Only replace if the template type is same as new type
+  auto Pderived = std::unique_ptr(new Derived());
+}
Index: docs/clang-tidy/checks/modernize-make-unique.rst

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Starting to look good i think.




Comment at: docs/ReleaseNotes.rst:88
 
+- New alias :doc:`abseil-make-unique
+  ` to :doc:`modernize-make-unique

Also add a new entry about the new option for `modernize-make-unique`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang updated this revision to Diff 190775.
axzhang added a comment.

Make fixes to the `UseLegacyFunction` option, renaming to `IgnoreListInit`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-make-unique.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-make-unique.rst
  test/clang-tidy/abseil-make-unique.cpp

Index: test/clang-tidy/abseil-make-unique.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-make-unique.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+
+namespace std {
+
+template 
+class default_delete {};
+
+template >
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr ) = delete;
+  unique_ptr(unique_ptr &) {}
+  ~unique_ptr() {}
+  type *() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr =(unique_ptr &&) { return *this; }
+  template 
+  unique_ptr =(unique_ptr &&) { return *this; }
+
+private:
+  type *ptr;
+};
+
+}  // namespace std
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+void expectPointer(std::unique_ptr p);
+
+std::unique_ptr makeAndReturnPointer() {
+  return std::unique_ptr(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique(0);
+}
+
+void Positives() {
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique(2);
+
+  // Non-primitive paramter
+  std::unique_ptr P2 = std::unique_ptr(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P2 = absl::make_unique(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique(3, 4);
+
+  // No arguments to new expression
+  std::unique_ptr P3 = std::unique_ptr(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P3 = absl::make_unique();
+
+  P3.reset(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P3 = absl::make_unique();
+
+  // Nested parentheses
+  std::unique_ptr P4 = std::unique_ptr((new int(3)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P4 = absl::make_unique(3);
+
+  P4 = std::unique_ptr(((new int(4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(4);
+
+  P4.reset((new int(5)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(5);
+
+  // With auto
+  auto P5 = std::unique_ptr(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: auto P5 = absl::make_unique();
+
+  {
+// No std
+using namespace std;
+unique_ptr Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: unique_ptr Q = absl::make_unique();
+
+Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: Q = absl::make_unique();
+  }
+
+  // Create the unique_ptr as a parameter to a function
+  expectPointer(std::unique_ptr(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: expectPointer(absl::make_unique());
+}
+
+void Negatives() {
+  // Only warn if explicitly allocating a new object
+  std::unique_ptr R = std::unique_ptr(returnPointer());
+  R.reset(returnPointer());
+
+  // Only replace if the template type is same as new type
+  auto Pderived = std::unique_ptr(new Derived());
+}
Index: docs/clang-tidy/checks/modernize-make-unique.rst

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang updated this revision to Diff 190676.
axzhang added a comment.

Updated diff with full context.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-make-unique.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-make-unique.rst
  test/clang-tidy/abseil-make-unique.cpp

Index: test/clang-tidy/abseil-make-unique.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-make-unique.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+
+namespace std {
+
+template 
+class default_delete {};
+
+template >
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr ) = delete;
+  unique_ptr(unique_ptr &) {}
+  ~unique_ptr() {}
+  type *() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr =(unique_ptr &&) { return *this; }
+  template 
+  unique_ptr =(unique_ptr &&) { return *this; }
+
+private:
+  type *ptr;
+};
+
+}  // namespace std
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+void expectPointer(std::unique_ptr p);
+
+std::unique_ptr makeAndReturnPointer() {
+  return std::unique_ptr(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique(0);
+}
+
+void Positives() {
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique(2);
+
+  // Non-primitive paramter
+  std::unique_ptr P2 = std::unique_ptr(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P2 = absl::make_unique(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique(3, 4);
+
+  // No arguments to new expression
+  std::unique_ptr P3 = std::unique_ptr(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P3 = absl::make_unique();
+
+  P3.reset(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P3 = absl::make_unique();
+
+  // Nested parentheses
+  std::unique_ptr P4 = std::unique_ptr((new int(3)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P4 = absl::make_unique(3);
+
+  P4 = std::unique_ptr(((new int(4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(4);
+
+  P4.reset((new int(5)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(5);
+
+  // With auto
+  auto P5 = std::unique_ptr(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: auto P5 = absl::make_unique();
+
+  {
+// No std
+using namespace std;
+unique_ptr Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: unique_ptr Q = absl::make_unique();
+
+Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: Q = absl::make_unique();
+  }
+
+  // Create the unique_ptr as a parameter to a function
+  expectPointer(std::unique_ptr(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: expectPointer(absl::make_unique());
+}
+
+void Negatives() {
+  // Only warn if explicitly allocating a new object
+  std::unique_ptr R = std::unique_ptr(returnPointer());
+  R.reset(returnPointer());
+
+  // Only replace if the template type is same as new type
+  auto Pderived = std::unique_ptr(new Derived());
+}
Index: docs/clang-tidy/checks/modernize-make-unique.rst

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:163
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == 
CXXNewExpr::ListInit) {
+return;

elide the braces



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:237
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == 
CXXNewExpr::ListInit) {
+return;

elide the braces



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:6
+abseil-make-unique
+
+

the  length needs to match the text above it


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:55
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+  UseLegacyFunction(Options.getLocalOrGlobal("UseLegacyFunction", false)) 
{}
 

I'm not sure it makes sense to look for global `UseLegacyFunction`?
It doesn't sound all that common.



Comment at: docs/clang-tidy/checks/modernize-make-unique.rst:52
+
+.. option:: UseLegacyFunction
+   

Name is too cryptic i'd say.
Maybe just `IgnoreInitListExprs`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please always upload all patches with full context (`=U9`)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang updated this revision to Diff 190559.
axzhang added a comment.

Apologies for the extended hiatus. I have changed the implementation to an 
alias to modernize-make-unique, and have added an extra option to 
modernize-make-unique that ignores C++11 features such as list initializations, 
to allow `absl::make_unique` to function correctly. Also, after looking more 
into `absl::WrapUnique`, it seems like this also is not really the function to 
use for brace initializations, so no extra functionality needs to be added to 
the existing modernize-make-unique check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tidy/modernize/MakeSmartPtrCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-make-unique.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-make-unique.rst
  test/clang-tidy/abseil-make-unique.cpp

Index: test/clang-tidy/abseil-make-unique.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-make-unique.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+
+namespace std {
+
+template 
+class default_delete {};
+
+template >
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr ) = delete;
+  unique_ptr(unique_ptr &) {}
+  ~unique_ptr() {}
+  type *() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr =(unique_ptr &&) { return *this; }
+  template 
+  unique_ptr =(unique_ptr &&) { return *this; }
+
+private:
+  type *ptr;
+};
+
+}  // namespace std
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+void expectPointer(std::unique_ptr p);
+
+std::unique_ptr makeAndReturnPointer() {
+  return std::unique_ptr(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique(0);
+}
+
+void Positives() {
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique(2);
+
+  // Non-primitive paramter
+  std::unique_ptr P2 = std::unique_ptr(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P2 = absl::make_unique(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique(3, 4);
+
+  // No arguments to new expression
+  std::unique_ptr P3 = std::unique_ptr(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P3 = absl::make_unique();
+
+  P3.reset(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P3 = absl::make_unique();
+
+  // Nested parentheses
+  std::unique_ptr P4 = std::unique_ptr((new int(3)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P4 = absl::make_unique(3);
+
+  P4 = std::unique_ptr(((new int(4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(4);
+
+  P4.reset((new int(5)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(5);
+
+  // With auto
+  auto P5 = std::unique_ptr(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: auto P5 = absl::make_unique();
+
+  {
+// No std
+using namespace std;
+unique_ptr Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: unique_ptr Q = absl::make_unique();
+
+Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: Q = absl::make_unique();
+  }
+
+  // Create the unique_ptr as a parameter to a function
+  expectPointer(std::unique_ptr(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use absl::make_unique instead 

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

>> In D55044#1329604 , @hokein wrote:
>>  In fact, the existing modernize-make-unique can be configured to support 
>> absl::make_unique, you'd just need to configure the check option 
>> MakeSmartPtrFunction to absl::make_unique (this is what we do inside google).
> 
> See https://reviews.llvm.org/D52670#change-eVDYJhWSHWeW (the 
> getModuleOptions() part) on how to do that

Thanks! Yes, it is trivial to create an alias check, but if we want to support 
`WrapUnique`, I don't think this is a right way to go.

In D55044#1330102 , @axzhang wrote:

> In D55044#1329604 , @hokein wrote:
>
> > In fact, the existing `modernize-make-unique` can be configured to support 
> > `absl::make_unique`, you'd just need to configure the check option 
> > `MakeSmartPtrFunction` to `absl::make_unique` (this is what we do inside 
> > google).
> >
> > The biggest missing part of the `modernize-make-unique` is 
> > `absl::WrapUnique`, I think we should extend `MakeSmartPtrCheck` class 
> > (maybe add hooks) to support it.
>
>
> What is the best way to extend `MakeSmartPtrCheck`? The behavior I want to 
> achieve is that `absl::WrapUnique` is suggested when brace initialization is 
> used, but `absl::make_unique` is used in all other cases.


I think we'd need to add more interfaces to `MakeSmartPtrCheck` allowing 
subclasses to inject their logic to handle different initialization styles of 
`new` expression.

- `MakeSmartPtrCheck::replaceNew` is the interesting place
- we may split `MakeSmartPtrCheck::replaceNew` implementation into different 
methods (e.g. `handleCallInit`, `handleListInit`) which are corresponding to 
handle different `new` cases
- from my experience,  handling brace initialization is tricky, there are lots 
of corner cases, you can see the comments in `MakeSmartPtrCheck::replaceNew`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang added a comment.

In D55044#1329604 , @hokein wrote:

> In fact, the existing `modernize-make-unique` can be configured to support 
> `absl::make_unique`, you'd just need to configure the check option 
> `MakeSmartPtrFunction` to `absl::make_unique` (this is what we do inside 
> google).
>
> The biggest missing part of the `modernize-make-unique` is 
> `absl::WrapUnique`, I think we should extend `MakeSmartPtrCheck` class (maybe 
> add hooks) to support it.


What is the best way to extend `MakeSmartPtrCheck`? The behavior I want to 
achieve is that `absl::WrapUnique` is suggested when brace initialization is 
used, but `absl::make_unique` is used in all other cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D55044#1329604 , @hokein wrote:

> In fact, the existing `modernize-make-unique` can be configured to support 
> `absl::make_unique`, you'd just need to configure the check option 
> `MakeSmartPtrFunction` to `absl::make_unique` (this is what we do inside 
> google).


See https://reviews.llvm.org/D52670#change-eVDYJhWSHWeW (the 
`getModuleOptions()` part) on how to do that

> The biggest missing part of the `modernize-make-unique` is 
> `absl::WrapUnique`, I think we should extend `MakeSmartPtrCheck` class (maybe 
> add hooks) to support it.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In fact, the existing `modernize-make-unique` can be configured to support 
`absl::make_unique`, you'd just need to configure the check option 
`MakeSmartPtrFunction` to `absl::make_unique` (this is what we do inside 
google).

The biggest missing part of the `modernize-make-unique` is `absl::WrapUnique`, 
I think we should extend `MakeSmartPtrCheck` class (maybe add hooks) to support 
it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/abseil/MakeUniqueCheck.h:29
+/// \endcode
+class MakeUniqueCheck : public modernize::MakeSmartPtrCheck {
+public:

Ah, so there is a check already, i missed that somehow.
If the `modernize::MakeSmartPtrCheck` is not sufficient to the needs, **it** 
needs to be extended.

You should be adding an **alias**, and this isn't the right way to do so.
E.g. see D53771 on how to do that.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked an inline comment as done.
EricWF added a comment.

I'm not sure how you're building this, but it doesn't link for me. We need to 
add a dependency on `clangTidyModernizeModule` since we're deriving from its 
`MakeSmartPtrCheck`.




Comment at: clang-tidy/abseil/MakeUniqueCheck.cpp:28
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  hasName("::std::unique_ptr"), templateArgumentCountIs(2),
+  hasTemplateArgument(

Eugene.Zelenko wrote:
> EricWF wrote:
> > Does this catch `std::__1::unique_ptr`, where `__1` is an inline namespace? 
> > (Either way, we should add a test)
> > 
> > Maybe it's better to first filter declarations using an `isInStdNamespace` 
> > matcher and then check for the name `unique_ptr`.
> Tests for other checks don't contain libc++ specific, so if problems existed 
> they were resolved long time ago.
Using versioning namespaces is not specific to libc++. But, as you suggested, 
this case appears to work.

I also see that this is just a copy of `modernize/MakeUniqueCheck.cpp`. Can we 
find a way to share the matcher between these two implementations?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/abseil/MakeUniqueCheck.cpp:28
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  hasName("::std::unique_ptr"), templateArgumentCountIs(2),
+  hasTemplateArgument(

EricWF wrote:
> Does this catch `std::__1::unique_ptr`, where `__1` is an inline namespace? 
> (Either way, we should add a test)
> 
> Maybe it's better to first filter declarations using an `isInStdNamespace` 
> matcher and then check for the name `unique_ptr`.
Tests for other checks don't contain libc++ specific, so if problems existed 
they were resolved long time ago.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: clang-tidy/abseil/MakeUniqueCheck.cpp:28
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  hasName("::std::unique_ptr"), templateArgumentCountIs(2),
+  hasTemplateArgument(

Does this catch `std::__1::unique_ptr`, where `__1` is an inline namespace? 
(Either way, we should add a test)

Maybe it's better to first filter declarations using an `isInStdNamespace` 
matcher and then check for the name `unique_ptr`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:93
+
+  Checks for instances of initializing a `unique_ptr` with a direct call to
+  `new` and suggests using `absl::make_unique` instead.

Please use double `, not single ones to hightlight language constructs. Same in 
documentation.



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:10
+Replaces initialization of smart pointers:
+\code
+  std::unique_ptr ptr = std::unique_ptr(new int(1));

Please use .. code-block:: c++. See other checks documentation as example.



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:19
+
+per the Abseil tips and guidelines at https://abseil.io/tips/126.

Please use hyperlink like:

The `Abseil Style Guide `_ discusses this issue in 
more detail.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

axzhang updated this revision to Diff 177972.
axzhang added a comment.

Per the suggestions of reviewers, change the check so that it uses 
modernize-make-smart-pointer instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/MakeUniqueCheck.cpp
  clang-tidy/abseil/MakeUniqueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-make-unique.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-make-unique.cpp

Index: test/clang-tidy/abseil-make-unique.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-make-unique.cpp
@@ -0,0 +1,130 @@
+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+
+// CHECK-FIXES: #include 
+
+namespace std {
+
+template 
+class default_delete {};
+
+template >
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr ) = delete;
+  unique_ptr(unique_ptr &) {}
+  ~unique_ptr() {}
+  type *() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr =(unique_ptr &&) { return *this; }
+  template 
+  unique_ptr =(unique_ptr &&) { return *this; }
+
+private:
+  type *ptr;
+};
+
+}  // namespace std
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+void expectPointer(std::unique_ptr p);
+
+std::unique_ptr makeAndReturnPointer() {
+  // Make smart pointer in return statement
+  return std::unique_ptr(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique(0);
+}
+
+void Positives() {
+  std::unique_ptr P1 = std::unique_ptr(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P1 = absl::make_unique(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique(2);
+
+  // Non-primitive paramter
+  std::unique_ptr P2 = std::unique_ptr(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P2 = absl::make_unique(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique(3, 4);
+
+  // No arguments to new expression
+  std::unique_ptr P3 = std::unique_ptr(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P3 = absl::make_unique();
+
+  P3.reset(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P3 = absl::make_unique();
+
+  // Nested parentheses
+  std::unique_ptr P4 = std::unique_ptr((new int(3)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr P4 = absl::make_unique(3);
+
+  P4 = std::unique_ptr(((new int(4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(4);
+
+  P4.reset((new int(5)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique(5);
+
+  // With auto
+  auto P5 = std::unique_ptr(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: auto P5 = absl::make_unique();
+
+  {
+// No std
+using namespace std;
+unique_ptr Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: unique_ptr Q = absl::make_unique();
+
+Q = unique_ptr(new int());
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use absl::make_unique instead [abseil-make-unique]
+// CHECK-FIXES: Q = absl::make_unique();
+  }
+
+  // Create the unique_ptr as a parameter to a function
+  expectPointer(std::unique_ptr(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: expectPointer(absl::make_unique());
+}
+
+void Negatives() {
+  // Only warn if explicitly allocating a new object
+  std::unique_ptr R = std::unique_ptr(returnPointer());
+  R.reset(returnPointer());
+
+  // Only replace if the template type is same as new type
+  auto Pderived = std::unique_ptr(new Derived());

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Marking both of these as "changes requested" to highlight the similarity of 
issues in them.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D55044#1312438 , @lebedev.ri wrote:

> Thanks for working on this.
>  Semi-duplicate of D50852  Please see 
> discussion there.
>  It should not be an abseil-specific check, the prefix (`std`,`abseil`), and 
> the function (`make_unique`) should be a config option, and it should likely 
> be a part of `modernize-use-auto`.


I agree here. Instead of reimplementing the functionality we should rather 
refactor the existing check and make it flexible. It is possible to register 
the `absl` version of it with a different default-configuration so the 
refactoring will be the correct one.




Comment at: test/clang-tidy/abseil-make-unique.cpp:43
+
+  std::unique_ptr b;
+  b.reset(new int(2));

All your test cases seem to create only a single `unique_ptr`, please add cases 
like

```
std::unique_ptr b1(new int(32)), b2(new int(42));
```
From my experience in rewriting decls, they should be excluded. It is very 
cumbersome to get it right in all cases (think pointers, pointer to pointers, 
const, ...) and `readability-isolcate-declaration` exists to split variable 
declarations first.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:70
 
+- New :doc:`abseil-make-unique
+  ` check.

Please use alphabetical order for new checks.



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:6
+
+Replaces unique pointers that are constructed with raw pointers with 
``absl::make_unique``, for leak-free dynamic allocation.
+

Please synchronize with Release Notes.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55044/new/

https://reviews.llvm.org/D55044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits