adding FileSkipped() to PPCallbacks

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,

file_skipped.patch (2.31 KB)

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

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

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

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.

Thanks for the review, Chris. Please see the new patch. I kept the
signature of FileSkipped() unchanged to be consistent with
FileChanged().

file_skipped2.patch (2.19 KB)

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

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,

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

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

file_skipped3.patch (2.69 KB)

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