[PATCH] D97196: [clang-tidy] Add new check 'bugprone-unhandled-exception-at-new'.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp:143
+  f_est_noexcept_dependent_used<true>();
+}
----------------
balazske wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > You have tests for placement new with `nothrow_t`, but I think other forms of placement new are also very interesting to test as those typically don't throw.
> > > 
> > > Additionally, perhaps tests where the allocation functions have been replaced by the user (such as a class allocation function)?
> > I realized that the case of user-defined constructor or allocation function allows to throw any kind of exception. So the check should be improved to handle this case: Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?
> > (And a function to collect all possible exceptions called from a function is needed, `ExceptionAnalyzer` seems usable.)
> It looks like that the user is free to define custom `operator new` and any constructor called that may throw any exception. Even in the "nothrow" case it is possible to use a constructor that may throw? If we would analyze every possible throwable exception that may come out from a new-expression, the checker would end up almost in a general checker that checks for uncaught exceptions. At least it is easy to extend.
> Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?

I think that's a better approach.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp:143
+  f_est_noexcept_dependent_used<true>();
+}
----------------
aaron.ballman wrote:
> balazske wrote:
> > balazske wrote:
> > > aaron.ballman wrote:
> > > > You have tests for placement new with `nothrow_t`, but I think other forms of placement new are also very interesting to test as those typically don't throw.
> > > > 
> > > > Additionally, perhaps tests where the allocation functions have been replaced by the user (such as a class allocation function)?
> > > I realized that the case of user-defined constructor or allocation function allows to throw any kind of exception. So the check should be improved to handle this case: Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?
> > > (And a function to collect all possible exceptions called from a function is needed, `ExceptionAnalyzer` seems usable.)
> > It looks like that the user is free to define custom `operator new` and any constructor called that may throw any exception. Even in the "nothrow" case it is possible to use a constructor that may throw? If we would analyze every possible throwable exception that may come out from a new-expression, the checker would end up almost in a general checker that checks for uncaught exceptions. At least it is easy to extend.
> > Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?
> 
> I think that's a better approach.
> Even in the "nothrow" case it is possible to use a constructor that may throw?

Certainly -- the `nothrow` syntax is specifying that the allocation cannot throw (it either returns a valid pointer to the allocated memory or null), not that the constructor cannot throw if the allocation succeeds.

Is the check intended to care about *allocations* that fail or just exceptions in general around `new` expressions? If it's allocations that fail, then I wouldn't worry about the ctor's exception specification. If exceptions in general, then I agree that we're talking about a general checker for all uncaught exceptions in some ways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97196