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

[Alp Toker]

On 20/01/2014 22:37, Rafael Espíndola wrote:
>>> 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.

I agree with Rafael. These facilities shouldn't be available to LLVM 
core or clang, because any use would be out of place in a compiler 
library. Features like that are best left to the embedding application 
-- it's better to put the effort into providing good programming 
interfaces that solve a problem.

FindProgramByName() is a good example of how things can go sour -- an 
emulation of shell path lookup added to support utilities like bugpoint, 
now used in lli and clang as a "fuzzy" alternative to getting the right 
semantics.

Alp.





>   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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-- 
http://www.nuanti.com
the browser experts