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

[Michael Spencer]

On Tue, Nov 23, 2010 at 4:46 PM, Chris Lattner <clattner at apple.com> wrote:
> On Nov 23, 2010, at 1:18 PM, Michael Spencer wrote:
>> 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.
>
> Looking great.  Some comments:
>
>
> +class file_status
> +{
> ...
> +  file_type type() const;
> +  void type(file_type v);
>
> Please use get_/set_ prefixes.  Naming getters and setters the same thing leads to confusing client code IMO.

OK.

> +error_code remove_filename(SmallVectorImpl<char> &path);
>
> I see this as getting confused with 'remove'.  Maybe 'remove' should be named remove_from_disk or something?  I think it would be good to pick some sort of uniform suffix and put them on everything that modifies the file system, e.g. copy_files_on_disk, create_directories_on_disk, exists_on_disk etc.
>
> Another way to handle this would be to put all the path manipulation stuff in sys::path, but put all the file system manipulation stuff in sys::fs.  sys::fs::remove() would be very clear to me.

OK.

> +error_code native(const Twine &path, SmallVectorImpl<char> &result);
>
> native isn't a verb, how about 'canonicalize'?

canonicalize exists. And the difference between the two is that
'native' is lexical, while 'canonicalize' acts on the file system.
They are now path::native and fs::canonicalize.

> +error_code current_path(SmallVectorImpl<char> &result);
>
> likewise, how about get_current_path?  similarly with the root_ names as well as filename, stem, parent_path etc.

Seems redundant and verbose to me. It also makes some names slightly
different from TR2/boost, which I think would be confusing. Obviously
different isn't a problem.

> +error_code is_valid(const Twine &path, bool &result);
>
> how about "is_valid_name" to make it clear that it isn't going to disk to see if the file is valid.

Uses path::is_valid.

> +/// @brief Get relative path.
> +///
> +/// @param path Input path.
> +/// @param result Set to the path starting after root_path if one exists,
> +///               otherwise "".
> +/// @results errc::success if result has been successfully set, otherwise a
> +///          platform specific error_code.
> +error_code relative_path(const Twine &path, StringRef &result);
>
> I don't understand this operation, an example would be great :)  It would be great to get examples for stem and extension as well.

I'll add more docs. In this case:

C:\hello\world => hello\world
foo/bar          => foo/bar
/foo/bar         => foo/bar

> +error_code set_read(const Twine &path, bool value);
> +error_code set_write(const Twine &path, bool value);
> +error_code set_execute(const Twine &path, bool value);
>
> Is the only way to set a mode to do:
>  setRead(X, true);
>  setWrite(X, true);
>
> If that turns into two syscalls, that seems wasteful.  Likewise with can_read/can_write.

These were in the set of functions directly copied over from the
sys::Path class. I (and the people working on filesystem) agree that
this isn't the best way to do it. I'm tempted to design a permissions
API, but LLVM has no need for that beyond RWX. However, for the sake
of syscalls, I could make the API flag based.

> +error_code is_empty(const Twine &path, bool &result);
>
> How about is_empty_file to make it obvious that this only applies to files, not directories?

Now fs::is_empty.

> +error_code space(const Twine &path, space_info &result);
>
> This could use a better name, get_disk_space would be better at least.

I agree with this. Changed.

> +error_code unique_path(const Twine &model, SmallVectorImpl<char> &result);
>
> This API is inherently racy unless it returns a newly created file.  I don't think we should have it.

I agree. I'll remove this and temp_directory_path and add a more
family friendly unique_file function (or some such name).

> +class directory_entry {
> +  SmallString<128> Path;
>
> I'm leery of directory_entry being this large.  While I don't know how it will be used from the API so far, I imagine that some clients will want to have vectors of these.  Given that, and that being so large will punish move construction, I'd suggest just making this contain an std::string.  However, that really depends on how you see it being used.  What do you think?

These are constructed once per iterator and reused for each iteration,
so std::string should be fine.

> Overall, looks great,
>
> -Chris

Thanks for all the comments. I'll make the changes you mentioned and
commit as per IRC and continue development in tree.

- Michael Spencer