[PATCH] Use -ivfsoverlay in ASTUnit

[Ben Langmuir]

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.



> 
>> 
>>> 
>>> 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.

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/20140414/a7d6930c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: astunit.patch
Type: application/octet-stream
Size: 14380 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140414/a7d6930c/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140414/a7d6930c/attachment-0001.html>