[llvm-commits] [PATCH] New Path API design.

[Michael Spencer]

On Mon, Nov 22, 2010 at 9:48 PM, Chris Lattner <clattner at apple.com> wrote:
>
> On Nov 20, 2010, at 3:06 PM, Michael Spencer wrote:
>
>>> +struct file_type {
>>> +  enum _ {
>>>
>>> Why _ instead of giving it a name?  I'd suggest file_type_kind.  We use "kind" for discriminators elsewhere.
>>
>> The enums are like this to emulate 'enum class' from C++0x. Users
>> never directly use the _ name, they use 'file_type::xyz'.
>
> Ok.
>
>>> +  int v_;
>>>
>>> Likewise, why v_? Particularly if this is public data it should have a good name and a doxygen comment.
>>
>> It should have been private. This is again just a helper to emulate
>> 'enum class'.
>
> Ok.
>
>>> +/// file_status - Represents the result of a call to stat and friends. It has
>>> +///               a platform specific member to store the result.
>>> +class file_status
>>> +{
>>>
>>> Seems fine so far.  Clang in particular is highly performance sensitive when it comes to stats and wants to reduce them wherever possible.  It really wants to do things like use "open + fstat" instead of "stat+open" since fstat is faster than stat.  This isn't a problem with your proposal, just pointing it out so that you're aware.
>>
>> Interesting. It should be simple enough to add a function that returns
>> both a raw_ostream and a file_status.
>
> Yep, I imagine that you'll find it when stuff starts converting over.  Just be conscientious about syscalls in clang :)
>
>
>>> +error_code make_absolute(SmallVectorImpl<char> &path);
>>>
>>> Please add doxygen comments to these with an example.  Also, what happens if an empty string is passed in?
>>
>> I'm going to fully document everything. An empty string would result
>> in the current directory.
>
> Thanks again Michael!
>
> -Chris

Attached are two patches. The first adds a new unit-test SystemTests
which has the TimeValue test and a new (empty) path test. The second
adds include/llvm/System/PathV2.h with documentation and the
modifications you mentioned.

- Michael Spencer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: path-v2.patch
Type: application/octet-stream
Size: 34610 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101123/5ea1eee8/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SystemTests.patch
Type: application/octet-stream
Size: 4234 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101123/5ea1eee8/attachment-0001.obj>