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

refactor: introduce ModifierTarget for better dealing with modifiers #4177

Merged
merged 5 commits into from
Oct 13, 2021

Conversation

SirYwell
Copy link
Collaborator

Previously, the getModifiers method had a isMethod boolean that was used to indicate that the modifiers are targetting a constructor or a method. Besides the vague naming, Java 17 introduces two new modifiers: sealed and non-sealed. Both of them require more context because their bits are used in different contexts too:

0x4000000
ExtraCompilerModifiers#AccIsDefaultConstructor
ExtraCompilerModifiers#AccBlankFinal
ExtraCompilerModifiers#AccNonSealed
0x10000000
ExtraCompilerModifiers#AccSealed
ExtraCompilerModifiers#AccPatternVariable
ExtraCompilerModifiers#AccOverriding

(this information was extracted with the ModifierConstantsCollector tool included in this PR).

One way to deal with that would be to add another boolean flag isType to check against, but I think it's a little bit clearer to use an enum. That makes it more readable and more expressive (and also more future proof).

I also added tests that help with implementing support for new modifiers.

@SirYwell SirYwell changed the title wip: refactor: Use enum for modifier target review: refactor: Use enum for modifier target Sep 22, 2021
@monperrus monperrus changed the title review: refactor: Use enum for modifier target review: refactor: introduce ModifierTarget for dealing with modifiers Sep 23, 2021
@monperrus
Copy link
Collaborator

Thanks a lot, that's super interesting. The overall concept LGTM and explicit metadata in enums is good.

We need to javadoc and maybe better name ModifierTarget.

What's the meaning of ModifierTarget's values? The acceptable / possible modifiers for each element kind?

@SirYwell
Copy link
Collaborator Author

We need to javadoc and maybe better name ModifierTarget.

What's the meaning of ModifierTarget's values? The acceptable / possible modifiers for each element kind?

I agree, I'm not super happy with the name. Another idea for a name was ModifierContext, but it's all a little bit vague. Basically it is meant to describe the place where the modifiers belong to (like *public* class X, void myMethod(*final* Parameter param), *private static final* int i = 42.

As for now, only the AccTransient and AccVarArgs overlapped, so it was enough to distinguish between methods and everything else.

The set of allowed modifiers for such target/conext is currently only used for the tests but it would be possible to add validation of which modifiers are accepted to the different implementations (e.g. throw an exception or just ignore it if someone tries to add the public modifier to a local variable).

@SirYwell SirYwell force-pushed the refactor/getModifiers branch from b735534 to b3dc472 Compare October 7, 2021 14:03
@SirYwell
Copy link
Collaborator Author

SirYwell commented Oct 7, 2021

I rebased and resolved the conflicts. As I was surprised by #4205 myself, it might be even easier to support sealed classes correctly. I think this PR is the only blocker then for now.

@monperrus monperrus changed the title review: refactor: introduce ModifierTarget for dealing with modifiers review: refactor: introduce ModifierTarget for better dealing with modifiers Oct 8, 2021
@monperrus
Copy link
Collaborator

LGTM. I cannot find a better name.

Let's javadoc ModifierTarget and we're done.

@SirYwell
Copy link
Collaborator Author

SirYwell commented Oct 9, 2021

I added some javadoc.

The enum is currently package private and only used for the JDTTreeBuilder to avoid more boolean flags on the getModifiers method. We can make it public in future, probably making it a property of CtModifiable and automatically checking if a modifier is valid in the CtModifierHandler. But I would consider that a separate feature (and we can still change the name then).

@monperrus
Copy link
Collaborator

thanks, we fix the conflict and merge.

@monperrus monperrus changed the title review: refactor: introduce ModifierTarget for better dealing with modifiers refactor: introduce ModifierTarget for better dealing with modifiers Oct 13, 2021
@monperrus monperrus merged commit aa26d0d into INRIA:master Oct 13, 2021
@monperrus
Copy link
Collaborator

Thanks a lot @SirYwell

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