gribozavr marked an inline comment as done. gribozavr added inline comments.
================ Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:226 +ExceptionAnalyzer::ExceptionInfo +ExceptionAnalyzer::analyzeBoilerplate(const T *Node) { + ExceptionInfo ExceptionList; ---------------- 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()`. 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