On Feb 14, 2014, at 8:53 AM, Argyrios Kyrtzidis <[email protected]> wrote:
> 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. 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. Ben > >> >> 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
