[llvm] r265068 - Add disk_space() to llvm::fs

[Mehdi Amini via llvm-commits]

> On Mar 31, 2016, at 4:35 PM, Rui Ueyama <ruiu at google.com> wrote:
> 
> On Thu, Mar 31, 2016 at 4:18 PM, Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
> 
>> On Mar 31, 2016, at 4:14 PM, Rui Ueyama <ruiu at google.com <mailto:ruiu at google.com>> wrote:
>> 
>> On Thu, Mar 31, 2016 at 4:05 PM, Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> Author: mehdi_amini
>> Date: Thu Mar 31 18:05:26 2016
>> New Revision: 265068
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=265068&view=rev <http://llvm.org/viewvc/llvm-project?rev=265068&view=rev>
>> Log:
>> Add disk_space() to llvm::fs
>> 
>> Summary: Adapted from Boost::filesystem.
>> (This is a reapply by reverting commit r265062 and fixing the WinAPI part)
>> 
>> Differential Revision: http://reviews.llvm.org/D18467 <http://reviews.llvm.org/D18467>
>> 
>> From: Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>>
>> 
>> Modified:
>>     llvm/trunk/include/llvm/Support/FileSystem.h
>>     llvm/trunk/lib/Support/Unix/Path.inc
>>     llvm/trunk/lib/Support/Windows/Path.inc
>> 
>> Modified: llvm/trunk/include/llvm/Support/FileSystem.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=265068&r1=265067&r2=265068&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=265068&r1=265067&r2=265068&view=diff>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/FileSystem.h (original)
>> +++ llvm/trunk/include/llvm/Support/FileSystem.h Thu Mar 31 18:05:26 2016
>> @@ -32,6 +32,7 @@
>>  #include "llvm/ADT/Twine.h"
>>  #include "llvm/Support/DataTypes.h"
>>  #include "llvm/Support/ErrorHandling.h"
>> +#include "llvm/Support/ErrorOr.h"
>>  #include "llvm/Support/TimeValue.h"
>>  #include <ctime>
>>  #include <iterator>
>> @@ -648,6 +649,17 @@ std::error_code identify_magic(const Twi
>> 
>>  std::error_code getUniqueID(const Twine Path, UniqueID &Result);
>> 
>> +/// @brief Get disk space usage information.
>> +///
>> +/// Note: Users must be careful about "Time Of Check, Time Of Use" kind of bug.
>> +/// Note: Windows reports results according to the quota allocated to the user.
>> +///
>> +/// @param Path Input path.
>> +/// @returns a space_info structure filled with the capacity, free, and
>> +/// available space on the device \a Path is on. A platform specific error_code
>> +/// is returned on error.
>> +ErrorOr<space_info> disk_space(const Twine Path);
>> 
>> I think you need to change the parameter type to `const Twine &`. 
> 
> 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!
> 
>> 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.
> 
> 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?
> 
> 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.

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

-- 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160331/87e0c535/attachment.html>