<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 11, 2014, at 2:16 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Very cool. A few high level questions:<div>- shouldn't the virtual file system go into llvm, not clang?<br></div></div></blockquote><div><br></div><div>I’m fine with putting this in llvm/Support instead of clang/Basic, but we’ll need to check with llvm-dev and the libsupport code owner. If you feel strongly about this I’ll send something to Chandler and llvm-dev.</div><br><blockquote type="cite"><div dir="ltr"><div>- seems to me like there is only one AbstractFileSystem that should be owned pretty top-level, so why use an IntrusiveRefCntPtr?</div></div></blockquote><div><br></div><div>This is based on an off-list discussion I had with Doug Gregor. To ensure modules get built with a consistent environment we want to share the file system among compiler instances. Also, the AbstractFileSystem may composed of several sub file-systems that can refer to each other. IntrusiveRefCntPtr seemed like a safer choice based on that.</div><br><blockquote type="cite"><div dir="ltr">
<div>- why not make all the function virtual? Doesn't seem to have real downsides, and would immediately enable fully virtualized file system implementations (seems fine to have default implementations for all the functions); if the plan is to have the functions state now to provide an easier migration path, I think it'd make sense to add a comment describing the end-state...</div>
<div><br></div></div></blockquote><div><br></div><div>Do you have any functions in mind that could benefit? Most of the non-virtual functions are built on top of the virtual status() operation. I would be concerned that an implementor might think it was a good idea to return something different from one of those query operations than what is in the Status object, which would certainly lead to problems.</div><br><blockquote type="cite"><div dir="ltr"><div>And a meta-thing:</div><div>- it would help me a lot if I was able to comment via phabricator - is there a specific reason you're not using it? (I'd be happy to help fix that reason if it makes reviews simpler - if the reason is not fixable, I'll just keep my mouth shut and download the patches from now on :)</div>
<div><br></div></div></blockquote><div><br></div><div>No particular reason - I’ll install arcanist and try to get you a phab review later today :)</div><div><br></div><div>Ben</div><div><br></div><blockquote type="cite"><div dir="ltr"><div>Otherwise looks like a nice first step...</div><div><br></div><div>Cheers,</div><div>/Manuel</div><div><br></div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Feb 10, 2014 at 11:32 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Doug et al.<br>
<br>
This is the first patch in the implementation of the virtual file system discussed here: <a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-February/035188.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-February/035188.html</a>. It is intended to include enough of the AbstractFileSystem interface to replace the use of llvm::sys::fs in FileManager. Incidentally, I’ve included a number of the convenience methods that are trivially implemented on top of the status object. When created by the CompilerInstance, FileManager will use a virtual file system created by the CI, but to avoid modifying every other user of the FileManager, in the absence of an explicit AbstractFileSystem to use, it will default to the real file system during construction.<br>
<br>
Additionally, I’ve included a stub implementation of the OverlayFileSystem, which the CompilerInstance uses. It is able to overlay files from a second file system, but will not merge directories correctly yet. There are no users that actually push a second file system yet (other than the included unit test).<br>
<br>
This patch is notably missing: the interfaces for recursive directory traversal, file system modification (create dir, rename, etc.), and any notion of thread safety.<br>
<span class="HOEnZb"><font color="#888888"><br>
Ben<br>
<br>
</font></span><br><br>
<br>
<br></blockquote></div><br></div>
</blockquote></div><br></body></html>