[PATCH] D110278: Adding doWithCleanup abstraction

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Frontend/FrontendAction.cpp:553-565
+  bool HasBegunSourceFile = false;
+  std::function<bool()> Operation = [&]() {
+    return BeginSourceFileImpl(CI, RealInput, HasBegunSourceFile);
+  };
+  std::function<void()> Cleanup = [&]() {
+    if (HasBegunSourceFile)
+      CI.getDiagnosticClient().EndSourceFile();
----------------
I guess this would be written as:
```
bool Success = BeginSourceFileImpl(CI, RealInput, HasBegunSourceFile))
if (!Success) {
  if (HasBegunSourceFile)
    CI.getDiagnosticClient().EndSourceFile();
  CI.clearOutputFiles(/*EraseFiles=*/true);
  CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
  setCurrentInput(FrontendInputFile());
  setCompilerInstance(nullptr);
}
return Success;
```
Which seems pretty legible - makes me unsure it's worth the complexity of building something like `doWithCleanup`, not that it's heinously complicated by any means - but all the traits stuff, etc.

& ideally/in general this'd usually be dealt with with some kind of RAII ownership model rather than such a strong stateful transition of an existing object that needs to be undone, rather than destroying an object.



================
Comment at: llvm/include/llvm/Support/Cleanup.h:56-59
+template <typename ErrT>
+ErrT doWithCleanup(
+    std::function<ErrT()> Fn, std::function<void()> CleanupFn,
+    std::function<bool(ErrT &)> IsFailure = &detail::isFailure<ErrT>) {
----------------
If this is going to be a template anyway, might as well use raw/generic/templated functor arguments and avoid the dynamic dispatch overhead of std::function? (or at least use llvm::function_ref)


================
Comment at: llvm/unittests/Support/CleanupTests.cpp:19
+  bool Result =
+      doWithCleanup<bool>([] { return true; }, [&] { CleanupRun = true; });
+  EXPECT_TRUE(Result);
----------------
Presumably doWithCleanup doesn't need to be called with an explicit template type argument most of the time/ever? It's probably good to test it in that way too, without passing the explicit template type argument.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110278/new/

https://reviews.llvm.org/D110278