[PATCH] Use -ivfsoverlay in ASTUnit

[Ben Langmuir]

On Apr 14, 2014, at 11:50 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:

> 
> On Apr 14, 2014, at 11:30 AM, Ben Langmuir <blangmuir at apple.com> wrote:
> 
>> 
>> On Apr 14, 2014, at 11:27 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>> 
>>> 
>>> On Apr 14, 2014, at 10:27 AM, Ben Langmuir <blangmuir at apple.com> wrote:
>>> 
>>>> 
>>>> On Apr 14, 2014, at 9:16 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>>> 
>>>>> 
>>>>> On Apr 14, 2014, at 8:45 AM, Ben Langmuir <blangmuir at apple.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On Apr 11, 2014, at 5:45 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> 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 <blangmuir at apple.com> 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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140415/6c05dc93/attachment.html>