On Apr 14, 2014, at 11:27 AM, Argyrios Kyrtzidis <[email protected]> wrote:
> > 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 ? Do we actually want to create a new VFS object (including parsing the VFS files)? It seems pointless when we already have one in ASTUnit’s file manager. > > 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. Sure, will do. > > 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
