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

[Rafael Espíndola]

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

Well, we now also test that no extra files were created. I agree that
this would be cleaner with a RAII that would delete the file, but it
should live in unittest/ unless there is a user somewhere else in
LLVM.

Having code in lib/Support that is only used in unittest is a very bad
practice IMHO.

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

If, and only if, there is a tool that needs to really recursively
delete a directory, then we it is reasonable to have a function that
does that. Even the cases we had in unittest and the potential example
you have seems better served by a RAII class that deletes a file or at
most one that deletes a list of known files.

It is hard to imagine that a compiler tool really needs to delete any
file that might be in a particular directory. It is a fairly dangerous
tool to have around. Consider for example that we stat before a remove
to make sure that we don't try to delete /dev/null for example. We
should also be careful if a cache directory is incorrectly set to a
non empty directory.

Cheers,
Rafael