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

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

Reply via email to