[PATCH] Some infrastructure work for virtual file system

[Manuel Klimek]

On Tue, Feb 11, 2014 at 5:20 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Feb 11, 2014, at 2:16 AM, Manuel Klimek <klimek at google.com> wrote:
>
> Very cool. A few high level questions:
> - shouldn't the virtual file system go into llvm, not clang?
>
>
> I’m fine with putting this in llvm/Support instead of clang/Basic, but
> we’ll need to check with llvm-dev and the libsupport code owner.  If you
> feel strongly about this I’ll send something to Chandler and llvm-dev.
>

Yep, I think this really belongs into llvm/Support.


> - seems to me like there is only one AbstractFileSystem that should be
> owned pretty top-level, so why use an IntrusiveRefCntPtr?
>
>
> This is based on an off-list discussion I had with Doug Gregor.  To ensure
> modules get built with a consistent environment we want to share the file
> system among compiler instances.  Also, the AbstractFileSystem may composed
> of several sub file-systems that can refer to each other.
>  IntrusiveRefCntPtr seemed like a safer choice based on that.
>

Hm, for composition both owning children and not owning children seems
fine, and for sharing the file system via compiler instances it seems to me
like the owner of the compiler instances would also own the file system.
Maybe it's that lacking knowledge of the future I tend to try to err on the
side of less "general" solutions; I cannot come up with a stronger
argument, so I guess it's fine :)


> - why not make all the function virtual? Doesn't seem to have real
> downsides, and would immediately enable fully virtualized file system
> implementations (seems fine to have default implementations for all the
> functions); if the plan is to have the functions state now to provide an
> easier migration path, I think it'd make sense to add a comment describing
> the end-state...
>
>
> Do you have any functions in mind that could benefit?  Most of the
> non-virtual functions are built on top of the virtual status() operation.
>  I would be concerned that an implementor might think it was a good idea to
> return something different from one of those query operations than what is
> in the Status object, which would certainly lead to problems.
>

Ah, I missed that little detail :) In that case, I really like the design!

One more thing: is there a specific reason to have the is_*(Status) methods
all static on the AbstractFileSystem instead of making them methods on
Status?


> And a meta-thing:
> - it would help me a lot if I was able to comment via phabricator - is
> there a specific reason you're not using it? (I'd be happy to help fix that
> reason if it makes reviews simpler - if the reason is not fixable, I'll
> just keep my mouth shut and download the patches from now on :)
>
>
> No particular reason - I’ll install arcanist and try to get you a phab
> review later today :)
>

Thanks!


>
> Ben
>
> Otherwise looks like a nice first step...
>
> Cheers,
> /Manuel
>
>
>
>
>
>
> On Mon, Feb 10, 2014 at 11:32 PM, Ben Langmuir <blangmuir at apple.com>wrote:
>
>> Hi Doug et al.
>>
>> This is the first patch in the implementation of the virtual file system
>> discussed here:
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-February/035188.html.
>>  It is intended to include enough of the AbstractFileSystem interface to
>> replace the use of llvm::sys::fs in FileManager. Incidentally, I’ve
>> included a number of the convenience methods that are trivially implemented
>> on top of the status object.  When created by the CompilerInstance,
>> FileManager will use a virtual file system created by the CI, but to avoid
>> modifying every other user of the FileManager, in the absence of an
>> explicit AbstractFileSystem to use, it will default to the real file system
>> during construction.
>>
>> Additionally, I’ve included a stub implementation of the
>> OverlayFileSystem, which the CompilerInstance uses.  It is able to overlay
>> files from a second file system, but will not merge directories correctly
>> yet.  There are no users that actually push a second file system yet (other
>> than the included unit test).
>>
>> This patch is notably missing: the interfaces for recursive directory
>> traversal, file system modification (create dir, rename, etc.), and any
>> notion of thread safety.
>>
>> Ben
>>
>>
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140211/3bbfb75a/attachment.html>