On Apr 14, 2014, at 11:30 AM, Ben Langmuir <[email protected]> wrote:
> > 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. Hmm, ok maybe this should accept a FileManager then (and it internally can pick up what it needs from it) but I’m fine as it is now. >> >> 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
