Page MenuHomePhabricator

[lldb][NFC] Remove strange bool parameter from Searcher::SearchCallback
ClosedPublic

Authored by teemperor on Oct 9 2019, 5:21 AM.

Details

Summary

The SearchCallback has a bool parameter that we always set to false, we never use in any callback implementation and that also changes its name
from one file to the other (either containing and complete). It was added in the original LLDB check in, so there isn't any history what
this was supposed to be, so let's just remove it.

Diff Detail

Event Timeline

teemperor created this revision.Oct 9 2019, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 5:21 AM

I just realised the same is true for addr (beside that it is consistently named). I'll remove this too after this has landed.

labath accepted this revision.Oct 9 2019, 5:33 AM

lol

This revision is now accepted and ready to land.Oct 9 2019, 5:33 AM
jingham accepted this revision.Oct 9 2019, 10:26 AM

One of the oddities of the Search -> Search callback stuff is that the Searcher's Filter might be narrower than the level at which the Search Callback wants to be invoked. The former is whatever the user actually wants limit the search to which is really undetermined, whereas the latter is the level that is convenient for implementing the callback, so there's no necessary linkage. That means that every time the callback finds a match, it has to check with the filter to see if the match falls within the filter. This parameter was supposed to be a way for the searcher to tell the callback that it's search scope exactly matched the filter, so the callback didn't need to check.

That would be useful if checking the filter ever showed up as a performance issue, but it never has. So it's fine to remove this.

I have no memory of adding the 'addr' parameter.

If you had a search that was guaranteed to only return one address per invocation, this would allow you to have the searcher coordinate gathering the results, rather than having the callback record them on its own. That might be another convenient usage pattern, but if we really wanted to do that we should have passed in a vector of addresses, since it seems hard to guarantee one-match per invocation. Since we've never actually wanted to handle searches this way, it seems fine to remove this parameter.

teemperor added a comment.EditedOct 10 2019, 3:17 AM

Thanks for the explanation! Doesn't seem like removing this breaks Swift, so I'll land this.

This revision was automatically updated to reflect the committed changes.