[cfe-dev] adding FileSkipped() to PPCallbacks

[Zhanyong Wan (λx.x x)]

On Fri, Apr 16, 2010 at 2:14 PM, Chris Lattner <clattner at apple.com> wrote:
>
> On Apr 16, 2010, at 2:09 PM, Zhanyong Wan (λx.x x) wrote:
>
>> Thanks for the review, Chris.  Please see the new patch.  I kept the
>> signature of FileSkipped() unchanged to be consistent with
>> FileChanged().
>
> Keeping the interfaces consistent is nice, but is it really worth it?  This will punish other PPCallbacks that don't care about this, like -MM mode and friends.  SourceMgr.createFileID isn't hyper-expensive, but it is good to avoid work that no other client at all needs.  Sinking this work into the implementation that cares makes sense to me.

If createFileID() is called here, the file being skipped has already
been processed earlier, which is a much more expensive operation.  So
do we really want to sacrifice the cleanness of the API for it?
Thanks,

> -Chris
>
>>
>> On Fri, Apr 16, 2010 at 1:34 PM, Chris Lattner <clattner at apple.com> wrote:
>>>
>>> On Apr 16, 2010, at 11:48 AM, Zhanyong Wan (λx.x x) wrote:
>>>
>>>> Hi Doug,
>>>>
>>>> Per our IRC chat yesterday, I'm extending PPCallbacks with a new
>>>> method FileSkipped() to notify a listener when a #include is skipped
>>>> due to header guard optimization.  Please see the attached patch and
>>>> let me know if this is the right direction.  (I verified that it works
>>>> for my use case.)  Also, how should I add a test for this?
>>>
>>> The .h file change look fine to me, but it isn't good to sink the check below SourceMgr.createFileID.  Skipping headers is a fairly hot path, so we don't want it to do additional work when ppcallbacks aren't being used.
>>>
>>> Please change the callback to take the FileEntry or some other information that doesn't require sinking the check below the potentially expensive work.
>>>
>>> -Chris
>>
>>
>>
>> --
>> Zhanyong
>> <file_skipped2.patch>
>
>



-- 
Zhanyong