Skip to content

optionally export private static fields of classes with friends #55

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrurogerz
Copy link
Collaborator

Purpose

Add a new command line option to IDS which enables it to export static, private fields of classes that have friend declarations. This fix specifically addresses feedback on llvm/llvm-project#136623 by automatically annotating AnalysisKey fields.

Overview

Adds a new --friendly-fields command line argument to IDS to enable exporting private fields for friend access. When this new mode is enabled, private fields are exported by VisitVarDecl if the containing class has at least one friend declaration.

NOTES:

  • I made this functionality a command line option because it doesn't seem like appropriate default behavior.
  • I did not add functionality to export private methods in the same conditions. Unlike just exporting private static fields, exporting methods over-exports a significant number of private methods when running against LLVM. It could easily be added as another command line option, but we don't currently have a use for it.

Validation

Added a new test case.
Ran tests on Windows, Fedora, and MacOS.

Copy link
Owner

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I don't think that this is a particularly well formed conceptually. These are private fields, not protected fields. I'm not sure if these should be exported as part of the ABI. I think that the user should be explicit about exposing the private fields.

@andrurogerz
Copy link
Collaborator Author

I don't think that this is a particularly well formed conceptually. These are private fields, not protected fields. I'm not sure if these should be exported as part of the ABI. I think that the user should be explicit about exposing the private fields.

Sure. But I could argue that private members are part of a class' interface if it has friends, which is basically what I was going for here. I admit it is a stretch because the members definitely aren't part of the overall public interface.

Regardless, I think you're arguing for something more along the lines of IDS callers explicitly specifying types they want exported regardless of access level. So in the example test I added, IDS would need to be explicitly told to export KeyType. Or are you suggesting something different?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants