Page MenuHomePhabricator

[clang-tidy] Add check bugprone-multiple-new-in-one-expression.
Needs ReviewPublic

Authored by balazske on Nov 28 2022, 1:05 AM.

Diff Detail

Event Timeline

balazske created this revision.Nov 28 2022, 1:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.Nov 28 2022, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 1:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Some of the code is copied from UnhandledExceptionAtNewCheck. Probably these checks can be merged into one NewExceptionHandlingCheck?

Thanks for this new check. Could you please link here results of this checker on som relevant open source projects?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.h
25

I think check is applicable to early versions too.

clang-tools-extra/docs/clang-tidy/checks/bugprone/multiple-new-in-one-expression.rst
9

Please follow 80 characters limit.

bitcoin, contour, qtbase, xerces was checked but nothing was detected by the check.

I'm really not sure this is the right approach to solve the problem. If the concern here is leaking on exceptions, then the goalpost should be flagging all calls to global new and recommend a smart pointer factory instead. I think that check could even be made as a cppcoreguideline check

CppCoreGuidelines rule r13-perform-at-most-one-explicit-resource-allocation-in-a-single-expression-statement is applicable for this problem. The current check is more restricted. If the check is moved to cppcoreguidelines it should check for any multiple new in a single expression and probably for other "resource allocation" that is not new (if there is such case at all that can not be changed to a safe function call or is not deprecated by other rules). Probably we can have an option to switch the behavior (any 2 new or the current way) and move the check to cppcoreguidelines.

This check is not about removing any use of new. It is only about the memory leak problem that is easy to detect and mentioned in the CppCoreGuidelines and CERT MEM52-CPP rules. If it is moved to the cppcoreguidelines module I would add an option that makes the check work like it works in this patch now, if it is turned off the check can find any expression where multiple new occurs (even when || and similar operators are used). My question is would the reviewers accept the check in the current form or should it be moved to cppcoreguidelines?