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.

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

Make it possible to extend ResourceIdentifier #55568

Open
Tracked by #55555
HosseinYousefi opened this issue Apr 25, 2024 · 5 comments
Open
Tracked by #55555

Make it possible to extend ResourceIdentifier #55568

HosseinYousefi opened this issue Apr 25, 2024 · 5 comments
Labels
analyzer-pkg-meta Issues related to package:meta area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-encodings Encoding related CFE issues.

Comments

@HosseinYousefi
Copy link
Member

ResourceIdentifier is currently not a final class. However, it won't behave correctly if we subclass it, and use the subclass as an annotation, since we're checking it based on the name.

I would like to be able to subclass it for jnigen, so I don't have to repeat certain things in the generated code. For example:

class JClassIdentifier extends ResourceIdentifier {
  const JClassIdentifier(String name, String somethingElse)
      : super('jnigen-$name-$somethingElse');
}

@JClassIdentifier('Foo', 'Bar') // instead of @ResourceIdentifier('jnigen-Foo-Bar')
late final JClass foobar;

@mosuem
cc/ @dcharkes

@HosseinYousefi HosseinYousefi added the analyzer-pkg-meta Issues related to package:meta label Apr 25, 2024
@lrhn
Copy link
Member

lrhn commented Apr 25, 2024

since we're checking it based on the name.

Yeah, there's the issue.

Can you even do:

const myResource = ResourceIdentifier(...);

@myResource
Some declaration...

?

@mosuem
Copy link
Member

mosuem commented Apr 26, 2024

Yes, the current check is name and library only. I like the idea of subclassing!

@lrhn lrhn added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Apr 26, 2024
@johnniwinther johnniwinther added the cfe-encodings Encoding related CFE issues. label Apr 26, 2024
@dcharkes
Copy link
Contributor

I think we should likely disallow implementing, because the metadata field could not exist in a subclass.

Also, we should specify what happens to subclasses specifying more fields. Are they reported in the resources.json? If they are reported, we should come up with an encoding that does not lead to potential conflicts.

class JClassIdentifier extends ResourceIdentifier {
  final String name;

  const JClassIdentifier(this.name)
      : super('jnigen');
}

This would currently lead to a class between name in the annotation and name of the method the annotation is on in how it's currently encoded.

@mosuem @HosseinYousefi @lrhn Any preferences on if the metadata field is special and the only one reported, or if we report all fields?

@HosseinYousefi
Copy link
Member Author

@lrhn
Unrelated question: why can we create a const string but not a const map from const constructor's arguments?

class JClassIdentifier extends ResourceIdentifier {
  const JClassIdentifier(String name, String somethingElse)
      : super(const {name: somethingElse}); // Error!
}

I assume it's because the const string and string have the same syntax but const map requires an extra const keyword. It would be nice to have something like "preferConst" so we could do

class JClassIdentifier extends ResourceIdentifier {
  const JClassIdentifier(String name, String somethingElse)
      : super(preferConst {name: somethingElse}); 
}

Since making {name: somethingElse} to be "preferConst" by default is a breaking change.

@HosseinYousefi
Copy link
Member Author

Any preferences on if the metadata field is special and the only one reported, or if we report all fields?

Since we can't quite fill the metadata as a map using the arguments of the subclass' const constructor, I prefer it if we report all fields if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-pkg-meta Issues related to package:meta area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-encodings Encoding related CFE issues.
Projects
None yet
Development

No branches or pull requests

5 participants