Page MenuHomePhabricator

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

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

Details

Summary

These SearchFilter methods all return true by their default
implementation:

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,
SearchFilterByModuleList.

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
lldb/include/lldb/Core/SearchFilter.h
102

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".

lldb/source/Core/SearchFilter.cpp
416

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.

542

Likewise.

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.

lldb/include/lldb/Core/SearchFilter.h
102

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

lldb/source/Core/SearchFilter.cpp
416

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.
lldb/include/lldb/Core/SearchFilter.h
102

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.

lldb/include/lldb/Core/SearchFilter.h
102

@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.