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

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:44-45
+    return BinOp->getOpcode() == BO_Assign &&
+           BinOp->getRHS()->IgnoreParenCasts() == E;
+
+  return isa<CallExpr, CXXConstructExpr>(ParentE);
----------------
In general, a value can be stored by passing it to a CompoundAssignOperator. This is not relevant in the current application (pointers do not appear on the RHS of compound assignment operators); but perhaps it's worth to mention this in a short comment in case someone wishes to reuse this function in some other checker.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/multiple-new-in-one-expression.rst:9-14
+C++ does often not specify the exact order of evaluation of the operands of an operator or arguments of a function.
+Therefore if a first allocation succeeds and a second fails, in an exception handler it is not possible to tell which allocation has failed and free the memory.
+Even if the order is fixed the result of a first ``new`` may be stored in a temporary location that is not reachable at the time when a second allocation fails.
+It is best to avoid any expression that contains more than one operator ``new`` call, if exception handling is used to check for allocation errors.
+Different rules apply for are the short-circuit operators ``||`` and ``&&`` and the ``,`` operator, where evaluation of one side must be completed before the other starts.
+Similarly, condition of a ``?`` operator is evaluated before the branches are evaluated.
----------------
Eugene.Zelenko wrote:
> Please follow 80 characters limit.
I think the handling of the case of the short-circuiting operators is ambiguous in this long paragraph: without the examples it's unclear whether the checker reports e.g. `f2(new A) || f1(new B);` as an error ("Even if the order is fixed ... It is best to avoid any expression that contains more than one operator ``new`` call ... " suggests that even these cases are reported). Try to restructure this description, perhaps by splitting it into two paragraphs: (1) quote/state all the relevant rules of C++ evaluation order (2) describe how "bad" ordering of the two allocation causes a memory leak.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138777