Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reintroduce accidentally deleted "protected" keyword in SymbolFileDWARF #69990

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Reintroduce accidentally deleted "protected" keyword in SymbolFileDWARF #69990

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class SymbolFileDWARF : public SymbolFileCommon {
friend class DebugMapModule;
friend class DWARFCompileUnit;
friend class DWARFDIE;
friend class DWARFASTParser;
friend class ::DWARFASTParserClang;

// Static Functions
Expand Down Expand Up @@ -321,6 +322,7 @@ class SymbolFileDWARF : public SymbolFileCommon {
m_file_index = file_index;
}

protected:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad for merging this, but I beg you not to reintroduce it. I originally intended to have an isolated patch just for this change, but I forgot about it after being able to build my project. In fact, I'm working on an out-of-tree dwarf parser plugin for the new language Mojo, and it requires accessing the methods that were originally marked as protected.
Logically, DWARFASTParserClang is able to exist thanks to being a friend class to this one, and my plugin needs the same visibility, but for obvious reasons it cannot be declared a friend of this class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which things are you using specifically? If you want to use them out of tree, we should think more carefully about which symbols actually make sense to expose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we merge it and then you can open your isolated patch with the methods you want to expose? Right now everything is public, including ivars.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ivars thing make sense. Could you at least just move the ivars into a protected or private section for now? If you strongly disagree, just go ahead with this patch and then I can solve it in a new patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the big issue on keeping everything public is that will will use those methods without realizing they were supposed to be private (like I did), and then it'll be a bigger problem to disentangle that later on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, after you merge this patch, I'll send another one exposing only what's needed. I'll also remove DWARFASTParserClang as a friend class, because pretty much what it uses is also what I need. I'll include @bulbazord as reviewer as well.

typedef llvm::DenseMap<const DWARFDebugInfoEntry *, Type *> DIEToTypePtr;
typedef llvm::DenseMap<const DWARFDebugInfoEntry *, lldb::VariableSP>
DIEToVariableSP;
Expand Down