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

Fix mapping stats not skipping anonymous classes #469

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

NebelNidas
Copy link
Member

@NebelNidas NebelNidas commented Sep 24, 2022

OroArmor and others added 3 commits September 24, 2022 16:32
Adds indexing of inner & enclosing classes, since it's the only way to properly detect an anonymous class
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Need to take some time to do a more indpeth review of this when im not tired.

Copy link
Contributor

@liach liach left a comment

Choose a reason for hiding this comment

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

These aren't really improvements; many things this patch does is already accomplishable, and what it tries to do is at great costs for it's done wrong

import cuchaz.enigma.translation.representation.entry.ClassEntry;

public class InnerClassIndex implements JarIndexer {
private Multimap<ClassDefEntry, InnerClassData> innerClasses = ArrayListMultimap.create();
Copy link
Contributor

Choose a reason for hiding this comment

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

This thing is wrong; we don't want to record one map entry for every MethodHandles.Lookup reference. To check if a class is an anonymous class, simply check its EnclosingMethods https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.7 which asm somehow calls "OuterClass" and describes it incorrectly

}
}

record OuterClassData(String owner, String name, String descriptor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These names are wrong names from ASM, should've been like EnclosingMethodData. Also hasEnclosingMethod is wrong too; it returns that whether the given class is enclosed in a regular method instead of <init> or <clinit> (read the italics in https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.7)

@NebelNidas
Copy link
Member Author

I'm going to commit the filtering of packages in a separate PR

@NebelNidas NebelNidas changed the title Mapping Stats improvements Fix mapping stats not skipping anonymous classes Sep 26, 2022
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.

5 participants