Skip to content

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?

@andrurogerz
Copy link
Collaborator Author

Not proceeding with this patch, per feedback.

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