Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.