On Feb 14, 2014, at 8:00 AM, Manuel Klimek <[email protected]> wrote:
> On Fri, Feb 14, 2014 at 4:15 PM, Ben Langmuir <[email protected]> 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. > > What am I missing? > /Manuel > > > > http://llvm-reviews.chandlerc.com/D2745 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
