<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Feb 14, 2014, at 8:53 AM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On Feb 14, 2014, at 8:00 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br><div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Feb 14, 2014 at 4:15 PM, Ben Langmuir<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="HOEnZb"><div class="h5"><br><br>================<br>Comment at: lib/Basic/VirtualFileSystem.cpp:62-83<br>@@ +61,24 @@<br>+<br>+ErrorOr<AbstractFileSystem::Status><br>+AbstractFileSystem::status(AbstractFileSystem::FileDescriptor FD) {<br>+ // FIXME: when we support virtual files, use information from the FD to lookup<br>+ // which AbstractFileSystem to perform this operation on.<br>+ return getRealFileSystem()->statusOfOpenFile(FD);<br>+}<br>+<br>+error_code AbstractFileSystem::getBufferForFile(<br>+ FileDescriptor FD, const llvm::Twine &Name,<br>+ llvm::OwningPtr<llvm::MemoryBuffer> &Result, int64_t FileSize,<br>+ bool RequiresNullTerminator) {<br>+ // FIXME: when we support virtual files, use information from the FD to lookup<br>+ // which AbstractFileSystem to perform this operation on.<br>+ return getRealFileSystem()->getBufferForOpenFile(FD, Name, Result, FileSize,<br>+ RequiresNullTerminator);<br>+}<br>+<br>+error_code AbstractFileSystem::close(AbstractFileSystem::FileDescriptor FD) {<br>+ // FIXME: when we support virtual files, use information from the FD to lookup<br>+ // which AbstractFileSystem to perform this operation on.<br>+ return getRealFileSystem()->closeOpenFile(FD);<br>+}<br>+<br>----------------<br></div></div><div class="">Ben Langmuir wrote:<br>> Manuel Klimek wrote:<br>> > I'd have expected this->*OpenFile(...) calls in those methods.<br>> These methods take a file descriptor as a parameter, so the file is already open.<br></div>Oh sorry, I think I misunderstood your comment.<br><br>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.<br></blockquote><div><br></div><div>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?</div><div><br></div><div>This design decision seems to currently just limit what you can do with an abstract file system.</div></div></div></div></blockquote><div><br></div><div>I think Ben’s point is that this operation is tied to the FileDescriptor object.</div><div><br></div><div>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.</div></div></div></blockquote></div><div><br></div><div>So you’re suggesting something like this:</div><div><br></div><div>class AbstactFileSystem {</div><div>…</div><div> class FileDescriptor {</div><div> public:</div><div> virtual ErrorOr<Status> status() = 0;</div><div> virtual error_code getBuffer(…) = 0;</div><div> virtual error_code close() = 0;</div><div> };</div><div>...</div><div>};</div><div><br></div><div>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.</div><div><br></div><div>Ben</div><div><br></div><div><blockquote type="cite"><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>What am I missing?</div><div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><br><br><a href="http://llvm-reviews.chandlerc.com/D2745" target="_blank">http://llvm-reviews.chandlerc.com/D2745</a></blockquote></div></div></div></blockquote></div></blockquote></div><br></body></html>