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

[Rui Ueyama via llvm-commits]

On Thu, Mar 31, 2016 at 4:39 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> 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>
> wrote:
>
>>
>> On Mar 31, 2016, at 4:14 PM, Rui Ueyama <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> 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
>>> 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
>>>
>>> From: Mehdi Amini <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
>>>
>>> ==============================================================================
>>> --- 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).
>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160331/b4a985ac/attachment.html>