Page MenuHomePhabricator

Refactor protected virtual functions from SymbolFile into new SymbolFileActual class.
ClosedPublic

Authored by yinghuitan on Apr 20 2022, 11:11 AM.

Details

Summary

This is a preparatory patch for https://reviews.llvm.org/D121631.
It refactors protected virtual member functions of SymbolFile
into a new SymbolFileActual class per suggestion in:
https://reviews.llvm.org/D121631

This will avoid the friendship declaration in that patch.

Diff Detail

Event Timeline

yinghuitan created this revision.Apr 20 2022, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 11:11 AM
Herald added a subscriber: mgorny. · View Herald Transcript
yinghuitan requested review of this revision.Apr 20 2022, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 11:11 AM

I might suggest we rename things like suggested in my inline comment, and then have the "SymbolFile.h" class just include the extra needed header file? Many of the changes in this diff would just go away. I am not a fan of the SymbolFileActual class nor having to switch to SymbolFileActual.h. I would almost prefer both SymbolFileActual and SymbolFile to stay in SymbolFile.h.

I will wait for Pavel to chime in as this was his suggestion.

lldb/include/lldb/Symbol/SymbolFile.h
45

Maybe just rename this to SymbolFileInterface, and then rename "SymbolFileActual" to "SymbolFile"?

lldb/include/lldb/Symbol/SymbolFileActual.h
18 ↗(On Diff #423971)

I might suggest we rename things like suggested in my inline comment, and then have the "SymbolFile.h" class just include the extra needed header file? Many of the changes in this diff would just go away.

Those would be consistent with some existing libraries (e.g. those which use the I prefix to denote pure interfaces), but if the goal is to reduce the number of changes, then I don't think it will help, as pretty much everything should still refer to the objects through the interface name. The name SymbolFileActual should pretty much only be used in the class declaration. but there are many more places that one would need to change from SymbolFile to SymbolFileInterface.

Instead of the name Actual, I might go for Common -- I'm thinking of the class as something that contains implementations "common" to many (but not necessarily all) symbol file classes.

I am not a fan of the SymbolFileActual class nor having to switch to SymbolFileActual.h. I would almost prefer both SymbolFileActual and SymbolFile to stay in SymbolFile.h.

Having SymbolFile.h include the other header would be strange, as the include would have to go to the bottom of the file. I did something like that recently, but only as a temporary transition aid.

However, I don't have a problem with having both classes be defined in the same header file. I don't know if there's any direct reference from SymbolFileOnDemand to SymbolFileActual. Ideally there wouldn't be, but if there is, then that class would also have to be defined in the same file -- also not a tragedy if the file does not get too big.

lldb/include/lldb/Symbol/SymbolFile.h
124–126

The comment probably not necessary. In this setup, there's no way that a non-virtual method can make sense, as there is nothing one could use to implement it.

430–440

What's up with the rest of the members. Can we move these too?
Any member that stays here runs the risk of going out of sync when we have two instances floating around.

Sounds good.

Here are the changes I plan to do:

  1. Rename SymbolFileActual => SymbolFileCommon.
  2. Inline SymbolFileCommon.h content into SymbolFile.h,
  3. Move all remaining member fields of SymbolFile class into SymbolFileCommon.
lldb/include/lldb/Symbol/SymbolFile.h
124–126

Sounds good.

430–440

Sure, I can move them.
I did not do in this patch initially because I want to avoid change too much code and get further feedback.

Address review comments.

labath accepted this revision.Apr 22 2022, 2:08 AM

Yes, that's pretty much what I had in mind. Thanks.

lldb/include/lldb/Symbol/SymbolFile.h
76

= default (or just delete altogether)

This revision is now accepted and ready to land.Apr 22 2022, 2:08 AM

Address review comment.

clayborg accepted this revision.Apr 25 2022, 10:36 AM
JDevlieghere accepted this revision.Apr 25 2022, 1:38 PM

Before I read the comments I was going to make the same suggestion as Greg, but this makes sense to limit churn. LGMT.