<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Mar 31, 2016 at 4:39 PM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div class="h5"><blockquote type="cite"><div>On Mar 31, 2016, at 4:35 PM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:</div><br><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_extra"><div class="gmail_quote">On Thu, Mar 31, 2016 at 4:18 PM, Mehdi Amini<span> </span><span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span><span> </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 style="word-wrap:break-word"><br><div><div><div><blockquote type="cite"><div>On Mar 31, 2016, at 4:14 PM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:</div><br><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_extra"><div class="gmail_quote">On Thu, Mar 31, 2016 at 4:05 PM, Mehdi Amini via llvm-commits<span> </span><span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span><span> </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">Author: mehdi_amini<br>Date: Thu Mar 31 18:05:26 2016<br>New Revision: 265068<br><br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project?rev=265068&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=265068&view=rev</a><br>Log:<br>Add disk_space() to llvm::fs<br><br>Summary: Adapted from Boost::filesystem.<br>(This is a reapply by reverting commit r265062 and fixing the WinAPI part)<br><br>Differential Revision:<span> </span><a href="http://reviews.llvm.org/D18467" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18467</a><br><br>From: Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>><br><br>Modified:<br>   <span> </span>llvm/trunk/include/llvm/Support/FileSystem.h<br>   <span> </span>llvm/trunk/lib/Support/Unix/Path.inc<br>   <span> </span>llvm/trunk/lib/Support/Windows/Path.inc<br><br>Modified: llvm/trunk/include/llvm/Support/FileSystem.h<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=265068&r1=265067&r2=265068&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=265068&r1=265067&r2=265068&view=diff</a><br>==============================================================================<br>--- llvm/trunk/include/llvm/Support/FileSystem.h (original)<br>+++ llvm/trunk/include/llvm/Support/FileSystem.h Thu Mar 31 18:05:26 2016<br>@@ -32,6 +32,7 @@<br> #include "llvm/ADT/Twine.h"<br> #include "llvm/Support/DataTypes.h"<br> #include "llvm/Support/ErrorHandling.h"<br>+#include "llvm/Support/ErrorOr.h"<br> #include "llvm/Support/TimeValue.h"<br> #include <ctime><br> #include <iterator><br>@@ -648,6 +649,17 @@ std::error_code identify_magic(const Twi<br><br> std::error_code getUniqueID(const Twine Path, UniqueID &Result);<br><br>+/// @brief Get disk space usage information.<br>+///<br>+/// Note: Users must be careful about "Time Of Check, Time Of Use" kind of bug.<br>+/// Note: Windows reports results according to the quota allocated to the user.<br>+///<br>+/// @param Path Input path.<br>+/// @returns a space_info structure filled with the capacity, free, and<br>+/// available space on the device \a Path is on. A platform specific error_code<br>+/// is returned on error.<br>+ErrorOr<space_info> disk_space(const Twine Path);<br></blockquote><div><br></div><div>I think you need to change the parameter type to `const Twine &`.<span> </span></div></div></div></div></div></blockquote><div><br></div></div></div><div>Oh your right, I was about to do it after your comment on phab, but then the windows bot yelled at me and I've been busy fixing the build to the point where I forgot. I'm glad you double-checked!</div><span><br><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_extra"><div class="gmail_quote"><div>As documented in Twine.h, Twine's implementation relies on the ability to store pointers to temporary stock objects which may be deallocated at the end of a statement, so having a Twine as a local variable (or a parameter) is not correct.</div></div></div></div></div></blockquote><div><br></div></span><div>You can't take ownership of the Twine, but this is not what happens here, is there a correctness issue (I see the inefficiency issue)? The twine will points to objects that are in the caller frame and won't be deallocated before the end of the call?</div></div></div></blockquote><div><br></div><div>I read the comment as there is a correctness issue, but I cannot point out the code which is not safe to use this way, so the comment may be wrong.</div></div></div></div></div></blockquote><div><br></div></div></div><div>If this is the comment "A Twine is not intended for use directly and should not be stored, its implementation relies on the ability to store pointers to temporary stack objects which may be deallocated at the end of a statement.", then this is what I'm talking about, this is the same issue as a StringRef (it references other data so you shouldn't store it unless you have guarantee on the lifetime of the referenced data).</div></div></div></blockquote><div><br></div><div>My understanding is that that is different from the StringRef case you mentioned here. I believe this comment is referring to the fact that temporary unnamed objects are destructed at end of each statement and Twine may have references to such objects.</div></div></div></div>