[PATCH] D59650: [NFC] ExceptionEscapeCheck: small refactoring

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:226
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::analyzeBoilerplate(const T *Node) {
+  ExceptionInfo ExceptionList;
----------------
gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > JonasToth wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > Please bikeshed on the name. I don't think this one is good.
> > > > > > Hmm, `analyzeGeneric`, `analyzeGeneral`, `abstractAnalysis`, `analyzeAbstract`, something good in these?
> > > > > > 
> > > > > > Given its private its not too important either ;)
> > > > > I'd suggest to simplify by changing `analyzeBoilerplate()` into a non-template, into this specifically:
> > > > > 
> > > > > ```
> > > > > ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::filterIgnoredExceptions(ExceptionInfo ExceptionList) {
> > > > >     if (ExceptionList.getBehaviour() == State::NotThrowing ||
> > > > >       ExceptionList.getBehaviour() == State::Unknown)
> > > > >     return ExceptionList;
> > > > > 
> > > > >   // Remove all ignored exceptions from the list of exceptions that can be
> > > > >   // thrown.
> > > > >   ExceptionList.filterIgnoredExceptions(IgnoredExceptions, IgnoreBadAlloc);
> > > > > 
> > > > >   return ExceptionList;
> > > > > }
> > > > > ```
> > > > > 
> > > > > And then call it in `analyze()`:
> > > > > 
> > > > > ```
> > > > > ExceptionAnalyzer::ExceptionInfo
> > > > > ExceptionAnalyzer::analyze(const FunctionDecl *Func) {
> > > > >   return filterIgnoredExceptions(analyzeImpl(Func));
> > > > > }
> > > > > ```
> > > > Hmm not really.
> > > > I intentionally did all this to maximally complicate any possibility of accidentally doing
> > > > something different given diferent entry point (`Stmt` vs `FunctionDecl`).
> > > > Refactoring it that way, via `filterIgnoredExceptions()` increases that risk back.
> > > > (accidentally omit that intermediate function, and ...)
> > > > (accidentally omit that intermediate function, and ...)
> > > 
> > > ... and tests should catch it.  No big drama.
> > > 
> > > Anyway, it is not as important.  I do think however that complicating the code this way is not worth the benefit.
> > That is kind of the point. The test would catch it if they would already exist.
> > If a new entry point is being added, the tests wouldn't be there yet.
> > This enforces that every entry point behaves the same way.
> > This enforces that every entry point behaves the same way.
> 
> Not really -- the new entry point can skip calling `analyzeDispatch` (and roll its own analysis) just like it can skip calling `filterIgnoredException()`.
Yep. I'm just hoping that that would look more out-of-place/be more noticeable than omitting some intermediate call.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59650