On Apr 14, 2014, at 10:27 AM, Ben Langmuir <[email protected]> wrote:
> > On Apr 14, 2014, at 9:16 AM, Argyrios Kyrtzidis <[email protected]> wrote: > >> >> On Apr 14, 2014, at 8:45 AM, Ben Langmuir <[email protected]> wrote: >> >>> >>> On Apr 11, 2014, at 5:45 PM, Argyrios Kyrtzidis <[email protected]> wrote: >>> >>>> IntrusiveRefCntPtr<LangOptions> LangOpts; >>>> IntrusiveRefCntPtr<DiagnosticsEngine> Diagnostics; >>>> + IntrusiveRefCntPtr<vfs::FileSystem> VFS; >>>> IntrusiveRefCntPtr<FileManager> FileMgr; >>>> IntrusiveRefCntPtr<SourceManager> SourceMgr; >>>> >>>> <…> >>>> >>>> DiagnosticsEngine &Diag, LangOptions &LangOpts, >>>> SourceManager &SourceMgr, FileManager &FileMgr, >>>> + vfs::FileSystem &VFS, >>>> >>>> Why do we need to keep the VFS separately, isn’t it owned by the >>>> FileManager ? >>>> >>> >>> No, it is conceptually owned by the CompilerInstance. The FileManager is a >>> user (albeit the only user right now). I say conceptually owned, because it >>> is a ref-counted object that doesn’t really have a single owner. >> >> So taking into account that it is a ref-counted object with no single owner, >> and FileManager already has reference to it (I think considering the >> FileManager as one of its owners makes sense IMO), why do we need it as a >> field in ASTUnit class and passing around as parameter when a FileManager >> parameter is already there ? > > Sure, I can drop the field. I will change ASTUnit, but keep CompilerInstance > the way it is for now, since you can reuse a VFS without reusing the > FileManager, so it makes sense to have a separate field in that case. > Updated patch attached. > > <astunit.patch> > >> >>> >>>> >>>> Would it be better if a >>>> IntrusiveRefCntPtr<vfs::FileSystem> FS; >>>> is part of FileSystemOptions ? And created at the time with get the >>>> FileSystemOptions for the compiler invocation ? >>>> >>>> It seems it would simplify a bunch of code. >>> >>> This would confuse the division of responsibilities between the compiler >>> invocation (which owns the FileSystemOptions) and the compiler instance. I >>> don’t think that the constructed VFS belongs in the compiler invocation. >>> I’m also not sure we should be reading and parsing VFS files during >>> command-line argument parsing. >> >> Fair enough, but on a related note, doesn’t the “VFSOverlayFiles” belong to >> the FileSystemOptions and not the HeaderSearchOptions ? > > Yeah that makes sense. I will move them in a separate patch, since it is not > directly relevant to this change. After you make this change how about changing: - AllocatedCXCodeCompleteResults(const FileSystemOptions& FileSystemOpts); + AllocatedCXCodeCompleteResults(const FileSystemOptions& FileSystemOpts, + vfs::FileSystem &VFS); And have it accept only ‘FileSystemOptions’ which will then use it to create a VFS object out of it ? Also nitpicking, isn’t better to pass the VFS here by ref-pointer so that the API is clear that it is accepts and retains a reference to it ? Otherwise it’s unclear if the AllocatedCXCodeCompleteResults will outlive the FileSystem you pass to it or not. Otherwise LGTM! > > Ben > >> >>> >>> Ben >>> >>>> >>>> On Apr 11, 2014, at 2:14 PM, Ben Langmuir <[email protected]> wrote: >>>> >>>>> Hi Dmitri and Argyrios, >>>>> >>>>> Could one (or both) of you take a look at my changes to the ASTUnit to >>>>> support the VFS? The VFS needs to be created for most/all of the >>>>> FileManagers that get created, and I’m a bit worried by the sheer number >>>>> of FileManager and SourceManager creations that I needed to plug up. >>>>> >>>>> Ben >>>>> >>>>> <astunit.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
