[PATCH] D138777: [clang-tidy] Add check bugprone-multiple-new-in-one-expression.

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:91
+                                      ExceptionType, ExceptionReferenceType)));
+  auto BadAllocCatchingTryBlock = cxxTryStmt(hasHandlerFor(CatchBadAllocType));
+
----------------
what about: ```catch(...)```


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:102
+          hasAnyArgument(
+              expr(HasNewExpr1, unless(equalsBoundNode("arg2"))).bind("arg1")),
+          hasAnyArgument(
----------------
this doesnt look valid, arg2 isn't known at this point yet, so this could be removed.
and this may not work for case like this:

```callSomething({new A,  new B});```  
Most probably this equalsBoundNode should be on HasNewExpr2 level, to avoid duplicated newExpr, not arguments of call

and image situation like this:

```
try {
  something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new int));
} catch(const std::bad_alloc&) {}
```
this in theory could also produce false-positive.

other issue is that first call could be anything, maloc, new, calloc, wzalloc, operator new(), it doesn't need to be just new.
You could try to use some simple way of checking this like, isBeforeInTransationUnit....

And this will also produce false-positive if we use `new(std::nothrow)` on second.
There are some "utils" to check sequence order, maybe would be good to investigate them.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:105
+              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+          hasAncestor(BadAllocCatchingTryBlock)),
+      this);
----------------
To be honest, I don't see any reason, how this try-catch would change anything, there can be one in parent function, and we going to heave a leak if we have try-catch or not.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:125
+      this);
+}
+
----------------
other issue I see is that same new could be matched multiple times by those Matchers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138777