[llvm] r198955 - Remove remove_all. A compiler has no need for recursively deleting a directory.

[Sean Silva]

On Fri, Jan 10, 2014 at 2:04 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> > Do we have a temporary directory RAII class or something? What is the
> > recommended way to handle the case where you have multiple temporary
> files
> > but for hygiene it is preferred to keep them together inside a temporary
> > directory? (isn't this what Clang does with its temporaries?)
>
> I considered creating a simple RAII class that takes a directory name
> and vends a getFile and getSubDir, but there were so few uses that I
> just used sys::remove.
>

If the intent of the code was to do a recursive remove, open-coding it is
just bad software engineering. For example, in one of the testcases you had
to add:

+
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive/a0/aa1"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive/a0/ab1"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive/a0"));
+  ASSERT_NO_ERROR(
+      fs::remove(Twine(TestDirectory) + "/recursive/dontlookhere/da1"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) +
"/recursive/dontlookhere"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive/pop/p1"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive/pop"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive/z0/za1"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive/z0"));
+  ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/recursive"));

This is a Single Point of Truth violation. The intent of the test fixture's
TearDown was to clean up after the test, and now the test is made more
complicated and more difficult to read/maintain since the user has to
manually remove all the files they create, when file removal isn't what is
being tested.


> If we do add such utility, it should live somewhere in unittests. We
> should not put stuff in Support for use just by unit tests.
>
> The closest thing I know in clang is the trimming of the module cache,
> but it is specific enough that I don't see a lot of opportunity for
> code sharing. Were you thinking of something else?
>

I was just thinking with the increasing number of tools that are living in
our trees, someone might run into a need for this (or a similar
functionality). For example, clang-modernize is working with inter-process
stuff, so it's not unrealistic to have to keep some files and then remove
them all (that's just an example off the top of my head).

It just seems like if someone does need this, having it there will save
them a good amount of time. The alternatives are either writing a crappy,
untested version inline or locally, or if they are aware of this patch
(unlikely), to revert it (and hopefully there won't be any bitrot they need
to fix). The situation is worse for an out of tree tool.

It just doesn't seem worth it for removing a couple dozen lines of
well-isolated and modular code from Support.

-- Sean Silva


>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140110/2b3ef125/attachment.html>