[PATCH] Some infrastructure work for virtual file system (now on phab)

Manuel Klimek klimek at google.com
Fri Feb 14 09:26:31 PST 2014


On Fri, Feb 14, 2014 at 6:19 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Feb 14, 2014, at 8:53 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com>
> wrote:
>
> On Feb 14, 2014, at 8:00 AM, Manuel Klimek <klimek at google.com> wrote:
>
> On Fri, Feb 14, 2014 at 4:15 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>
>>
>>
>> ================
>> Comment at: lib/Basic/VirtualFileSystem.cpp:62-83
>> @@ +61,24 @@
>> +
>> +ErrorOr<AbstractFileSystem::Status>
>> +AbstractFileSystem::status(AbstractFileSystem::FileDescriptor FD) {
>> +  // FIXME: when we support virtual files, use information from the FD
>> to lookup
>> +  // which AbstractFileSystem to perform this operation on.
>> +  return getRealFileSystem()->statusOfOpenFile(FD);
>> +}
>> +
>> +error_code AbstractFileSystem::getBufferForFile(
>> +    FileDescriptor FD, const llvm::Twine &Name,
>> +    llvm::OwningPtr<llvm::MemoryBuffer> &Result, int64_t FileSize,
>> +    bool RequiresNullTerminator) {
>> +  // FIXME: when we support virtual files, use information from the FD
>> to lookup
>> +  // which AbstractFileSystem to perform this operation on.
>> +  return getRealFileSystem()->getBufferForOpenFile(FD, Name, Result,
>> FileSize,
>> +
>> RequiresNullTerminator);
>> +}
>> +
>> +error_code AbstractFileSystem::close(AbstractFileSystem::FileDescriptor
>> FD) {
>> +  // FIXME: when we support virtual files, use information from the FD
>> to lookup
>> +  // which AbstractFileSystem to perform this operation on.
>> +  return getRealFileSystem()->closeOpenFile(FD);
>> +}
>> +
>> ----------------
>> Ben Langmuir wrote:
>> > Manuel Klimek wrote:
>> > > I'd have expected this->*OpenFile(...) calls in those methods.
>> > These methods take a file descriptor as a parameter, so the file is
>> already open.
>> Oh sorry, I think I misunderstood your comment.
>>
>> The reason we don't call this->*OpenFile in here is that the
>> FileDescriptor object (when we create one) should give us the
>> AbstractFileSystem that we need to call the method on.  If we have a
>> virtual file system composed of several AbstractFileSystems overlaid on top
>> of each other, we shouldn't have to search for which file system owns the
>> open file, since by opening it we already figured that out and can save it
>> in the file descriptor.
>>
>
> I think I don't understand that from a design point of view - if we have a
> file system implementation that "multiplexes" filesystems, I think it
> should be *its* respsonsibility how to multiplex it. What are the reasons
> we'd want that in the base class?
>
> This design decision seems to currently just limit what you can do with an
> abstract file system.
>
>
> I think Ben’s point is that this operation is tied to the FileDescriptor
> object.
>
> Ben, does it make sense to move such operations to a wrapper class of a
> file descriptor ? Seems a bit less confusing; it’s a bit weird to have
> AbstractFileSystem delegate to another AbstractFileSystem, it seems it
> should be a pure abstract interface.
>
>
> So you’re suggesting something like this:
>
> class AbstactFileSystem {
>>   class FileDescriptor {
>   public:
>     virtual ErrorOr<Status> status() = 0;
>     virtual error_code getBuffer(…) = 0;
>     virtual error_code close() = 0;
>   };
> ...
> };
>
> Manuel, does this address your concerns?  I mistakenly thought the above
> would mean a lot of changes to users of file descriptors, but since
> FileManager is hiding them anyway, that’s not a concern.
>

Yes, that would be the other option that I'd had in mind that would make
sense. I don't quite understand yet why the simpler design of having
AbstractFileSystem's non-virtual methods all be implemented in terms of the
virtual methods, and have implementations of AbstractFileSystem (like
RealFileSystem) do the work if necessary, is not enough; I'm very sure I'm
missing something, but I cannot put my finger on it yet...

Cheers,
/Manuel


>
> Ben
>
>
>
> What am I missing?
> /Manuel
>
>
>>
>>
>> http://llvm-reviews.chandlerc.com/D2745
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140214/15f313d3/attachment.html>


More information about the cfe-commits mailing list