[NFC] ExceptionEscapeCheck: small refactoring

[lebedev.ri]

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.

[JonasToth]

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

[lebedev.ri]

In D59650#1438515, @JonasToth wrote:

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.

[JonasToth]

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

[lebedev.ri]

Keep templated function out of the public interface.

In D59650#1438603, @JonasToth wrote:

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]

clang-tidy/utils/ExceptionAnalyzer.cpp

226

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

[JonasToth]

clang-tidy/utils/ExceptionAnalyzer.cpp

226

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

Given its private its not too important either ;)

[gribozavr]

clang-tidy/utils/ExceptionAnalyzer.cpp

208

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

226

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]

Address some nits.

clang-tidy/utils/ExceptionAnalyzer.cpp

226

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]

clang-tidy/utils/ExceptionAnalyzer.cpp

226

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

[lebedev.ri]

@gribozavr thank you for the review!

@baloghadamsoftware any comments?

clang-tidy/utils/ExceptionAnalyzer.cpp

226

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]

clang-tidy/utils/ExceptionAnalyzer.cpp

226

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]

clang-tidy/utils/ExceptionAnalyzer.cpp

226

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

[lebedev.ri]

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

clang-tidy/utils/ExceptionAnalyzer.h

public:
 
   ExceptionAnalyzer() = default;
 
   void ignoreBadAlloc(bool ShallIgnore) { IgnoreBadAlloc = ShallIgnore; }
   void ignoreExceptions(llvm::StringSet<> ExceptionNames) {
     IgnoredExceptions = std::move(ExceptionNames);
   }
 
-  ExceptionInfo analyze(const FunctionDecl *Func);
+  template <typename T> ExceptionInfo analyze(const T *Node);
 
 private:
   ExceptionInfo
   throwsException(const FunctionDecl *Func,
                   llvm::SmallSet<const FunctionDecl *, 32> &CallStack);
   ExceptionInfo
   throwsException(const Stmt *St, const ExceptionInfo::Throwables &Caught,
                   llvm::SmallSet<const FunctionDecl *, 32> &CallStack);
 
+  template <typename T>
+  void analyze(const T *Node, ExceptionInfo &ExceptionList);
+
   bool IgnoreBadAlloc = true;
   llvm::StringSet<> IgnoredExceptions;
   std::map<const FunctionDecl *, ExceptionInfo> FunctionCache;
 };
+
+extern template ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::analyze<FunctionDecl>(const FunctionDecl *Func);
+
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H

clang-tidy/utils/ExceptionAnalyzer.cpp

if (const auto *Throw = dyn_cast<CXXThrowExpr>(St)) {
     for (const Stmt *Child : St->children()) {
       ExceptionInfo Excs = throwsException(Child, Caught, CallStack);
       Results.merge(Excs);
     }
   }
   return Results;
 }
 
-ExceptionAnalyzer::ExceptionInfo
-ExceptionAnalyzer::analyze(const FunctionDecl *Func) {
-  ExceptionInfo ExceptionList;
-
+template <>
+void ExceptionAnalyzer::analyze(const FunctionDecl *Func,
+                                ExceptionInfo &ExceptionList) {
   // Check if the function has already been analyzed and reuse that result.
   if (FunctionCache.count(Func) == 0) {
     llvm::SmallSet<const FunctionDecl *, 32> CallStack;
     ExceptionList = throwsException(Func, CallStack);
 
     // Cache the result of the analysis. This is done prior to filtering
     // because it is best to keep as much information as possible.
     // The results here might be relevant to different analysis passes
     // with different needs as well.
     FunctionCache.insert(std::make_pair(Func, ExceptionList));
   } else
     ExceptionList = FunctionCache[Func];
+}
+
+template <typename T>
+ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::analyze(const T *Node) {
+  ExceptionInfo ExceptionList;
+
+  analyze<T>(Node, 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;
 }
+
+template ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::analyze<FunctionDecl>(const FunctionDecl *Func);
+
 } // namespace utils
 } // namespace tidy
 
 } // namespace clang