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