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

[Chris Lattner]

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.



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


+error_code native(const Twine &path, SmallVectorImpl<char> &result);

native isn't a verb, how about '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.


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


+/// @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.


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



+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?


+error_code space(const Twine &path, space_info &result);

This could use a better name, get_disk_space would be better at least.


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



+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?


Overall, looks great,

-Chris