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
