On Feb 11, 2014, at 2:16 AM, Manuel Klimek <[email protected]> wrote:
> Very cool. A few high level questions: > - shouldn't the virtual file system go into llvm, not clang? 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. > - seems to me like there is only one AbstractFileSystem that should be owned > pretty top-level, so why use an IntrusiveRefCntPtr? 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. > - 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... > 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. > And a meta-thing: > - 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 :) > No particular reason - I’ll install arcanist and try to get you a phab review later today :) Ben > Otherwise looks like a nice first step... > > Cheers, > /Manuel > > > > > > > On Mon, Feb 10, 2014 at 11:32 PM, Ben Langmuir <[email protected]> wrote: > Hi Doug et al. > > This is the first patch in the implementation of the virtual file system > discussed here: > http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-February/035188.html. 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. > > 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). > > This patch is notably missing: the interfaces for recursive directory > traversal, file system modification (create dir, rename, etc.), and any > notion of thread safety. > > Ben > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
