On Fri, Feb 14, 2014 at 6:19 PM, Ben Langmuir <[email protected]> wrote:
> > 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. > 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 > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
