This is an archive of the discontinued LLVM Phabricator instance.

[NFC] ExceptionEscapeCheck: small refactoring
ClosedPublic

Authored by lebedev.ri on Mar 21 2019, 9:25 AM.

Details

Summary

D59466 wants to analyse the Stmt, and ExceptionEscapeCheck does not
have that as a possible entry point.
This simplifies addition of Stmt analysis entry point.

Diff Detail

Event Timeline

lebedev.ri created this revision.Mar 21 2019, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 9:25 AM
Herald added a subscriber: rnkovacs. · View Herald Transcript

Why not having normal overloads? The analysis for Stmt is implemented with the private methods. Explicit template specialization is a bit overkill and so easily understood (but not too complex in this case either).l

Why not having normal overloads? The analysis for Stmt is implemented with the private methods. Explicit template specialization is a bit overkill and so easily understood (but not too complex in this case either).l

The alternative is to add something like:

ExceptionAnalyzer::ExceptionInfo
ExceptionAnalyzer::analyze(const Stmt *Stmt) {
  ExceptionInfo ExceptionList;

  llvm::SmallSet<const FunctionDecl *, 32> CallStack;
  ExceptionList = throwsException(Func, CallStack);

  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;
}

.. which ends up duplicating all of the boilerplate (blacklist in particular)
from ExceptionAnalyzer::analyze(const FunctionDecl *Func).

Looks like pointless code duplication that is easily avoidable.

Looks like pointless code duplication that is easily avoidable.

True, but I would prefer the refactoring to be private then (so the template stuff with the boilerplate) and a simple overloading interface dispatching to the templated stuff.
In the end I think the public interface is cleaner, easier to understand and not so easily misused (as the template would accept everything in principle, and SFINAE would be worse).

Keep templated function out of the public interface.

Looks like pointless code duplication that is easily avoidable.

True, but I would prefer the refactoring to be private then (so the template stuff with the boilerplate) and a simple overloading interface dispatching to the templated stuff.
In the end I think the public interface is cleaner, easier to understand and not so easily misused (as the template would accept everything in principle, and SFINAE would be worse).

Right. That looks a bit better. Works for me, thank you!

lebedev.ri marked an inline comment as done.Mar 21 2019, 2:36 PM
lebedev.ri added inline comments.
clang-tidy/utils/ExceptionAnalyzer.cpp
227

Please bikeshed on the name. I don't think this one is good.

JonasToth added inline comments.Mar 21 2019, 2:41 PM
clang-tidy/utils/ExceptionAnalyzer.cpp
227

Hmm, analyzeGeneric, analyzeGeneral, abstractAnalysis, analyzeAbstract, something good in these?

Given its private its not too important either ;)

gribozavr added inline comments.Mar 22 2019, 5:36 AM
clang-tidy/utils/ExceptionAnalyzer.cpp
211

I'd suggest to make it a non-template and call it also analyzeImpl() and return ExceptionInfo by value.

227

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));
}
lebedev.ri marked 3 inline comments as done.

Address some nits.

clang-tidy/utils/ExceptionAnalyzer.cpp
227

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 ...)

gribozavr accepted this revision.Mar 22 2019, 6:49 AM
gribozavr marked an inline comment as done.
gribozavr added inline comments.
clang-tidy/utils/ExceptionAnalyzer.cpp
227

(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.

This revision is now accepted and ready to land.Mar 22 2019, 6:49 AM
lebedev.ri marked an inline comment as done.Mar 22 2019, 6:52 AM

@gribozavr thank you for the review!

@baloghadamsoftware any comments?

clang-tidy/utils/ExceptionAnalyzer.cpp
227

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.

gribozavr marked an inline comment as done.Mar 22 2019, 7:35 AM
gribozavr added inline comments.
clang-tidy/utils/ExceptionAnalyzer.cpp
227

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().

lebedev.ri marked an inline comment as done.Mar 22 2019, 7:39 AM
lebedev.ri added inline comments.
clang-tidy/utils/ExceptionAnalyzer.cpp
227

Yep. I'm just hoping that that would look more out-of-place/be more noticeable than omitting some intermediate call.

I'm gonna go with post-commit review here, if @baloghadamsoftware will have any notes, although this now looks rather uncontroversial.

This revision was automatically updated to reflect the committed changes.