lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
@gribozavr thank you for the review!
@baloghadamsoftware any 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:
> > > 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.
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
