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

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

Thanks, I think these will be useful helpers to have! I think we do need to consider the API though, rather than being too anchored on the existing callsites.



================
Comment at: llvm/utils/unittest/support/llvm-test-helper.h:1
+//===-- llvm-test-helper.cpp ------------------------------------*- C++ -*-===//
+//
----------------
Missing header guard


================
Comment at: llvm/utils/unittest/support/llvm-test-helper.h:1
+//===-- llvm-test-helper.cpp ------------------------------------*- C++ -*-===//
+//
----------------
sammccall wrote:
> Missing header guard
Existing test helpers associated with support libraries live in llvm/Testing/Support, in namespace `llvm`. Can we follow that?

(Whereas the code in this directory is mostly third-party stuff).



================
Comment at: llvm/utils/unittest/support/llvm-test-helper.h:17
+
+struct ScopedDir {
+  SmallString<128> Path;
----------------
While making these more publicly visible, we should be a bit more careful about API:

I think they should mention "temporary" somewhere.
What about `TempDir`? I think the RAII nature can be clear from the usage and from the documentation.

We should consider hiding `Path`, avoiding assignment (including accidental - many sys::path functions take a mutable SmallString as inout param).
I think `StringRef TempDir::path()` is probably clearer than the StringRef operator, which we should consider dropping.
Another option I've had success with is `std::string TempDir::path(StringRef rel="")` which allows you to construct relative paths fluently.

We should add a doc comment for the class.


================
Comment at: llvm/utils/unittest/support/llvm-test-helper.h:46
+  SmallString<128> Path;
+  ScopedLink(const Twine &To, const Twine &From) {
+    Path = From.str();
----------------
this signature doesn't seem ergonomic for cross-platform tests (note that the current users are all `#ifdef LLVM_ON_UNIX`), because of the "/" vs "\" distinction.
If a pattern like `ScopedLink Link(RealPath, Dir.path("foo/bar"))` is expected, I'd suggest weakening `const Twine&` to `StringRef`. (Use of Twines for paths is IMO a historical mistake that we don't need to propagate further).

It's also not particularly obvious to me whether "To" is the link target or the link being written. Maybe Target/Link?


================
Comment at: llvm/utils/unittest/support/llvm-test-helper.h:61
+
+struct ScopedFile {
+  SmallString<128> Path;
----------------
the overloaded constructor is pretty confusing here: one creates a random filename based on a template and writes an empty file, the other wants the full path to be passed in and sets the contents.

If both are needed, I'd suggest something like:
```
class ScopedFile {
public:
  ScopedFile(StringRef Path, StringRef Contents="");
  static ScopedFile withUniquePath(StringRef NameModel="%%%%%%%%%%%%");
};
``` 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83228