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

================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:91
+                                      ExceptionType, ExceptionReferenceType)));
+  auto BadAllocCatchingTryBlock = cxxTryStmt(hasHandlerFor(CatchBadAllocType));
+
----------------
PiotrZSL wrote:
> what about: ```catch(...)```
`hasHandlerFor` checks for it (and it is used in the test).


================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:102
+          hasAnyArgument(
+              expr(HasNewExpr1, unless(equalsBoundNode("arg2"))).bind("arg1")),
+          hasAnyArgument(
----------------
PiotrZSL wrote:
> 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.
I am not an expert in how AST matchers work exactly, but this code works with 
the provided tests and the results look correct. I did not experience that two 
`new` in the same argument is matched, this is why the 
`unless(equalsBoundNode(...))` is added. The "arg1" and "arg2" nodes are the 
direct expressions in the function call, not descendants, and a `new` in the 
same argument (any descendant) is not matched twice in this way. Otherwise this 
would show up in failed tests.

The code `something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new 
int))` should produce warning (it may be possible that result of the first `new 
int` is not passed to `shared_ptr` before the other `new int` is called that 
fails, good solution is use of `std::make_shared` in such case). The test code 
`(void)f(g(new A), new B);` is somewhat similar AST, the `new A` should be 
found in all cases because `hasDescendant` is used at `HasNewExpr1` and 2.

Probably `unless(equalsBoundNode("arg2"))` can be removed, it is enough to have 
one of these checks.

`InitListExpr` is not handled by the current code, I need to add this case (it 
has fixed evaluation order).


================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:102
+          hasAnyArgument(
+              expr(HasNewExpr1, unless(equalsBoundNode("arg2"))).bind("arg1")),
+          hasAnyArgument(
----------------
balazske wrote:
> PiotrZSL wrote:
> > 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.
> I am not an expert in how AST matchers work exactly, but this code works with 
> the provided tests and the results look correct. I did not experience that 
> two `new` in the same argument is matched, this is why the 
> `unless(equalsBoundNode(...))` is added. The "arg1" and "arg2" nodes are the 
> direct expressions in the function call, not descendants, and a `new` in the 
> same argument (any descendant) is not matched twice in this way. Otherwise 
> this would show up in failed tests.
> 
> The code `something(std::shared_ptr<int>(new int), std::shared_ptr<int>(new 
> int))` should produce warning (it may be possible that result of the first 
> `new int` is not passed to `shared_ptr` before the other `new int` is called 
> that fails, good solution is use of `std::make_shared` in such case). The 
> test code `(void)f(g(new A), new B);` is somewhat similar AST, the `new A` 
> should be found in all cases because `hasDescendant` is used at `HasNewExpr1` 
> and 2.
> 
> Probably `unless(equalsBoundNode("arg2"))` can be removed, it is enough to 
> have one of these checks.
> 
> `InitListExpr` is not handled by the current code, I need to add this case 
> (it has fixed evaluation order).
The check only works with memory allocation failures that throw exceptions, but 
really only with `new`, this is the most often used. Functions like `malloc` do 
not throw exceptions.


================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:105
+              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+          hasAncestor(BadAllocCatchingTryBlock)),
+      this);
----------------
PiotrZSL wrote:
> 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.
The problem is that it is difficult to check if the parent function has a 
try-catch block, and the function can be called in different ways. This can 
results in false negatives. But without any try-catch block the program may not 
use exception handling at all (a failed `new` terminates the program), and it 
can be false positive to emit any warnings.


================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:125
+      this);
+}
+
----------------
PiotrZSL wrote:
> other issue I see is that same new could be matched multiple times by those 
> Matchers
This may be possible but I think that if the same warning is found multiple 
times it is shown only once, otherwise every case found by the matchers is a 
valid warning case even if the same `new` is involved.


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