adding FileSkipped() to PPCallbacks

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

adding FileSkipped() to PPCallbacks

Zhanyong Wan (λx.x x)
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?

Thanks,
--
Zhanyong

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

file_skipped.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Douglas Gregor
On Apr 16, 2010, at 11:48 AM, Zhanyong Wan (λx.x x) wrote:
> 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?

Looks good. I have no idea how to test this, since we don't have any way for preprocessor callbacks to show up in any of Clang's output. It's okay to commit something this small without a test.

        - Doug
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Zhanyong Wan (λx.x x)
On Fri, Apr 16, 2010 at 12:21 PM, Douglas Gregor <[hidden email]> wrote:
> On Apr 16, 2010, at 11:48 AM, Zhanyong Wan (λx.x x) wrote:
>> 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?
>
> Looks good. I have no idea how to test this, since we don't have any way for preprocessor callbacks to show up in any of Clang's output. It's okay to commit something this small without a test.

Thanks, Doug.  I don't have the right to commit.  Would you commit the
patch for me?  Thanks,

--
Zhanyong

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Chris Lattner
In reply to this post by Zhanyong Wan (λx.x x)

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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Abramo Bagnara-2
In reply to this post by Zhanyong Wan (λx.x x)
Il 16/04/2010 20:48, Zhanyong Wan (λx.x x) ha scritto:
> 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?

We have encountered a similar problem concerning the lack of PPCallbacks
for skipped excluded conditional blocks.

Do you see some problems in adding such a callback?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Chris Lattner

On Apr 16, 2010, at 1:40 PM, Abramo Bagnara wrote:

> Il 16/04/2010 20:48, Zhanyong Wan (λx.x x) ha scritto:
>> 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?
>
> We have encountered a similar problem concerning the lack of PPCallbacks
> for skipped excluded conditional blocks.
>
> Do you see some problems in adding such a callback?

Adding a callback for skipped #if blocks would be fine.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Zhanyong Wan (λx.x x)
In reply to this post by Chris Lattner
Thanks for the review, Chris.  Please see the new patch.  I kept the
signature of FileSkipped() unchanged to be consistent with
FileChanged().

On Fri, Apr 16, 2010 at 1:34 PM, Chris Lattner <[hidden email]> 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

file_skipped2.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Chris Lattner

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.

-Chris

>
> On Fri, Apr 16, 2010 at 1:34 PM, Chris Lattner <[hidden email]> 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>


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Zhanyong Wan (λx.x x)
On Fri, Apr 16, 2010 at 2:14 PM, Chris Lattner <[hidden email]> 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 <[hidden email]> 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Chris Lattner

On Apr 16, 2010, at 2:38 PM, Zhanyong Wan (λx.x x) wrote:

> On Fri, Apr 16, 2010 at 2:14 PM, Chris Lattner <[hidden email]> 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?

I'm pushing back because the preprocessor is a particularly performance sensitive part of the compiler, and this adds no value for any current clients.  I think the cost should be sunk into your client, even if it makes its implementation a little less elegant.

-Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Zhanyong Wan (λx.x x)
Please take a look at the new patch, which doesn't call createFileID()
for a skipped file.  Thanks!

On Fri, Apr 16, 2010 at 4:41 PM, Chris Lattner <[hidden email]> wrote:

>
> On Apr 16, 2010, at 2:38 PM, Zhanyong Wan (λx.x x) wrote:
>
>> On Fri, Apr 16, 2010 at 2:14 PM, Chris Lattner <[hidden email]> 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?
>
> I'm pushing back because the preprocessor is a particularly performance sensitive part of the compiler, and this adds no value for any current clients.  I think the cost should be sunk into your client, even if it makes its implementation a little less elegant.
>
> -Chris


--
Zhanyong

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

file_skipped3.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: adding FileSkipped() to PPCallbacks

Chris Lattner

On Apr 19, 2010, at 12:38 PM, Zhanyong Wan (λx.x x) wrote:

> Please take a look at the new patch, which doesn't call createFileID()
> for a skipped file.  Thanks!

Looks good, committed in r101813, thanks!

-Chris

>
> On Fri, Apr 16, 2010 at 4:41 PM, Chris Lattner <[hidden email]> wrote:
>>
>> On Apr 16, 2010, at 2:38 PM, Zhanyong Wan (λx.x x) wrote:
>>
>>> On Fri, Apr 16, 2010 at 2:14 PM, Chris Lattner <[hidden email]> 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?
>>
>> I'm pushing back because the preprocessor is a particularly performance sensitive part of the compiler, and this adds no value for any current clients.  I think the cost should be sunk into your client, even if it makes its implementation a little less elegant.
>>
>> -Chris
>
>
>
> --
> Zhanyong
> <file_skipped3.patch>


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev