Page MenuHomePhabricator

[lldb][nfc] remove overriden funcs with default impl

Authored by kwk on Apr 3 2020, 3:10 AM.



These SearchFilter methods all return true by their default

virtual bool ModulePasses(const FileSpec &spec);
virtual bool ModulePasses(const lldb::ModuleSP &module_sp);
virtual bool AddressPasses(Address &addr);
virtual bool CompUnitPasses(FileSpec &fileSpec);
virtual bool CompUnitPasses(CompileUnit &compUnit);

That's why I've documented the default behavior and remove the overrides
(except for AddressPasses) in these SearchFilter-subclasses which all just
repeated the default implementation: SearchFilterByModule,

Diff Detail

Event Timeline

kwk created this revision.Apr 3 2020, 3:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 3:10 AM
kwk edited the summary of this revision. (Show Details)Apr 3 2020, 5:55 AM
jankratochvil added inline comments.Apr 3 2020, 5:56 AM

I am not against it but FYI in a paragraph above there is already written: The default implementation is "Everything Passes."
English: IMO missing "returns".


This comment will get lost. Maybe you could keep this override just due to the FIXME comment. Or keep the FIXME comment some other way there. Or file a Bugzilla tracking Bug instead.
It is there since: Initial checkin of lldb code from internal Apple repo.



kwk updated this revision to Diff 255022.Apr 4 2020, 4:11 AM
  • fix comment
kwk updated this revision to Diff 255024.Apr 4 2020, 4:25 AM
kwk marked 3 inline comments as done.

Re-added AddressPasses with FIXME comment

kwk updated this revision to Diff 255026.Apr 4 2020, 4:31 AM
kwk edited the summary of this revision. (Show Details)
kwk added a subscriber: lattner.

Had some problem with my arc diff reporting that another git was running which it wasn't so I had to update the diff by hand with a bit of trouble.


Thank you for the missing "returns". I'd like to keep the comment.


I've checked the code myself to find out where it was coming from and found out that the FIXME is there for almost 10 years. I wonder if @lattner still remembers why it was there or if it can be removed. By the nature of the other default implementations below, I wonder if this was accidentally a left-over or should be put into SearchFilterByModule::CompUnitPasses as well?

kwk added a comment.Apr 4 2020, 4:33 AM

@jankratochvil for now I've re-added the AddressPasses functions and the FIXME comment. Can you approve?

jankratochvil accepted this revision.Apr 4 2020, 8:51 AM

LGTM, thanks for the cleanup.

In D77376#1961410, @kwk wrote:

so I had to update the diff by hand with a bit of trouble.

I was told here already it is better to use git diff -U999999999 as otherwise there is now Context not available. during this review. The line 416 comments are even not accessible in the diff now.

or should be put into SearchFilterByModule::CompUnitPasses as well?

CompUnitPasses is using SymbolContext::comp_unit which from my DWZ experience is many times not available. So for filtering by Modules the AddressPasses should be a more safe solution IMO.

This revision is now accepted and ready to land.Apr 4 2020, 8:51 AM
lattner removed a subscriber: lattner.Apr 4 2020, 10:37 AM
labath accepted this revision.Apr 6 2020, 2:38 AM
labath added a subscriber: labath.
labath added inline comments.

You can keep _a_ comment if you want, but this comment is even internally redundant. I don't see a need to mention overriding, saying "The default (base?) implementation returns true" should be clear enough.

kwk updated this revision to Diff 255300.Apr 6 2020, 5:40 AM
  • Simplify comments
kwk marked 2 inline comments as done.Apr 6 2020, 5:42 AM

@jankratochvil thanks for the tip.
@labath, addressed your comments and will now push it.


@labath. That is fine. I've changed the comment.

This revision was automatically updated to reflect the committed changes.
kwk marked an inline comment as done.