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

[Chris Lattner]

On Nov 19, 2010, at 11:56 AM, Michael Spencer wrote:

> Attached is my design for a new filesystem library to replace sys::Path. It is
> based on the TR2 filesystem proposal, which is based on boost. The interface
> looks similar, but has been significantly changed to remove exceptions.
> 
> These are the main changes:
> * Use error_code instead of filesystem_error exceptions. This is a significant
>  change to the API, which is why...
> * Functions always place their results into reference arguments and return an
>  error_code. The alternative of returning the result and then the error_code
>  as a function argument has also been considered; however, this allows for
>  function chains in which we loose the original error_code by the time the
>  statement is over.
> 
> * Remove the actual path class. Instead Twine, StringRef, and SmallString are
>  used depending on the argument. This makes the main part of the path API
>  stateless, and removes all that std::string allocation and copying we had
>  before.

This sounds really great Michael,

> The only LLVM specific functions that remain have kept their name, but have
> adopted the new return error_code semantics.
> 
> These are:
> error_code GetSystemLibraryPaths(SmallVectorImpl<SmallString<64> > &result);
> error_code GetBitcodeLibraryPaths(SmallVectorImpl<SmallString<64> > &result);
> error_code FindLibrary(Twine short_name, SmallVectorImpl<char> &result);
> error_code GetMainExecutable(const char *argv0, void *MainAddr,
>                             SmallVectorImpl<char> &result);

Makes sense.  These are not hot functions, but SmallVectorImpl<SmallString<64> > probably isn't a good idea, I'd recomment SmallVectorImpl<std::string>.  Many paths are longer than 64 characters, and a doubly nested smallvector is going to be very large on the stack.

> The rest of the old Path functions are no longer needed except for the map and
> unmap functions which I really need to take a look at for what I'll need in the
> object library in addition to what they already provide.
> 
> My plan is to first get an agreement over the API. Then commit the header. Then
> the implementations. The final and longest step will be actually transitioning
> over to the use of this API.

Sounds great!

Comments on the header:

+/*
+This is my design for a new filesystem library to replace sys::Path. It is based

This should have a normal file header of the standard style we use.


+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/System/system_error.h"

A bunch of these can be replaced with forward declarations.

+namespace path { // maybe filesystem?

I think that path is a good name, normal code would look like sys::path::make_absolute(xyz) for example.  If you don't like path, I'd suggest 'fs'.

+struct file_type {
+  enum _ {

Why _ instead of giving it a name?  I'd suggest file_type_kind.  We use "kind" for discriminators elsewhere.

Another issue is that this uses lower_case_stl_conventions.  It seems likely that we're about to get coding convention changes (see discussion on cfe-dev) that require UpperCaseWords conventions.

+  int v_;

Likewise, why v_? Particularly if this is public data it should have a good name and a doxygen comment.


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

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

Otherwise, looks great!

-Chris