<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jan 10, 2014 at 2:04 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>> Do we have a temporary directory RAII class or something? What is the<br>

> recommended way to handle the case where you have multiple temporary files<br>
> but for hygiene it is preferred to keep them together inside a temporary<br>
> directory? (isn't this what Clang does with its temporaries?)<br>
<br>
</div>I considered creating a simple RAII class that takes a directory name<br>
and vends a getFile and getSubDir, but there were so few uses that I<br>
just used sys::remove.<br></blockquote><div><br></div><div>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:</div>
<div><br></div><div><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px"><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/a0/aa1"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/a0/ab1"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/a0"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px"><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+      fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(Twine(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">TestDirectory) + "/recursive/dontlookhere/da1")</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">);</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/dontlookhere"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/pop/p1"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/pop"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/z0/za1"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive/z0"));</span><br style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">
<span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">+  ASSERT_NO_ERROR(fs::</span><span class="" style="font-family:arial,sans-serif;font-size:13px">remove</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">(</span><span style="color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px">Twine(TestDirectory) + "/recursive"));</span><br>
</div><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

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

<div><br></div><div>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.</div>
<div><br></div><div>It just doesn't seem worth it for removing a couple dozen lines of well-isolated and modular code from Support.</div><div><br></div><div>-- Sean Silva</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>