On Feb 11, 2014, at 12:23 PM, Argyrios Kyrtzidis <[email protected]> wrote:
> On Feb 10, 2014, at 2: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 >> >> <vfs-part1.patch> >> > > > Looks good! > > To add on the nitpicking.. :-) > > + /// In addition to providing the usual stat information, this class > provides > + /// \p Name and \p RealName fields, which can be used by clients that need > + /// to differentiate between the name that this object was looked up by, > + /// and the real name of this object on disk (if there is one). > > Could you clarify what should happen if there is no real path (it's a virtual > file), is RealName == Name or is it empty ? RealName == “” I’ll update the comment. > > >>> >>> I just thought that we might need to resolve symlinks ourselves, >>> here's why. Suppose that we have a real FS: >>> >>> /foo >>> /foo/bar/bar.h >>> /foo/baz/baz.h -> symlink to ../bar/bar.h >>> >>> Then we push an overlay over /foo/bar. If we rely on OS-level open(), >>> then opening baz/baz.h will open bar.h from the real FS, but it is >>> hidden by the overlay, thus we have an inconsistency. >> >> Good catch. It looks like the overlay file system will need to stat every >> path prefix and then resolve links itself when it finds them. That’s >> unpleasant, but doable. Looks like we’ll need to add a read link function >> to sys::fs… which might be a pain on Windows. I’m reading the documentation >> on Windows file system reparse points, and it’s not nearly as easy as >> calling ::readlink. > > I'm concerned about the performance implications of stat'ing everything and > trying to resolve symlinks, and I'm not sure how bad the issue will be in > practice. > Could we defer investigating into this for after we have the simpler case > working ? With caching I don’t think the performance will be a big problem, but we will have to see. I’m fine with deferring this. > > On a related note, what about using a filename with different casing from > what it was declared in the vfs ('myfoo.h' vs 'MyFoo.h'). > Since the overlay defers to other file systems, I don’t think it needs to care. When we add a file system for vfs files, we probably need an option to do case-insensitive matching. I’m not sure if that is a property of the host or just a property of the filesystem though… Ben _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
