[PATCH] D83228: [llvm] [unittests] Remove some temporary files after they're not needed

[Sergej Jaskiewicz via Phabricator via llvm-commits]

broadwaylamb added inline comments.


================
Comment at: llvm/include/llvm/Testing/Support/SupportHelpers.h:122
+  ///               llvm::sys::fs::createUniqueDirectory.
+  explicit TempDir(StringRef Name, bool Unique = false) {
+    std::error_code EC;
----------------
sammccall wrote:
> the bool param that changes the meaning of the previous param is a little cryptic (including at callsites).
> It also makes the common case of "unique temporary directory that I don't care about the name of
> 
> You could consider `TempDir(Name)` and `TempDir::unique()`. (Specifying the model doesn't seem all that useful?).
> 
> Up to you.
The name of the unique directory helps identify what went wrong if that directory isn't cleaned up. This is exactly how I've figured out which test cases need to use RAII objects.

For example, if we see that a directory isn't cleaned up, we can tell by its name which test case could crash and cause that directory to not be cleaned up.


================
Comment at: llvm/include/llvm/Testing/Support/SupportHelpers.h:158
+  template <typename... T>
+  SmallString<128> path(StringRef component, T &&... rest) const {
+    SmallString<128> Result(Path);
----------------
sammccall wrote:
> to avoid a bit of template magic, you can just accept a path separated by `/` and call native on it before append. I tend to find this more readable at the callsites that multiple params, up to you.
Good point, I'll do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83228