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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits