On Apr 14, 2014, at 11:50 AM, Argyrios Kyrtzidis <[email protected]> wrote:
> > 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. As discussed offline, we will just reuse the ASTUnit’s FileManager because it is ref-counted now. Committed as r206309 with this fix. Ben > >>> >>> 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
