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

ASM integration in TypesLoaderVisitor #86

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

ASM integration in TypesLoaderVisitor #86

wants to merge 16 commits into from

Conversation

rpau
Copy link
Owner

@rpau rpau commented Apr 12, 2018

This patch replaces reflection by ASM and increases
the performace almost a 20%. ASM parses binary files and
allows to extract the minimum amount of information that
is required to analyze.

@coveralls
Copy link

coveralls commented Apr 12, 2018

Coverage Status

Coverage increased (+0.08%) to 86.127% when pulling 49c6e2a on rpau-asm into d7da813 on master.

}
}

public Boolean isAnonymous() {
Copy link
Collaborator

@cal101 cal101 Apr 12, 2018

Choose a reason for hiding this comment

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

Primitive return instead of wrapper?

return actualName;
}

public Boolean isPrivate() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return primitive?

if (!actualName.contains(File.separator)){
return null;
}
return getActualName().substring(0, getActualName().lastIndexOf(File.separator) + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really file separator? Or internal class name with "/"?

}

outputStream.flush();
byte[] var5 = outputStream.toByteArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inline.

Copy link
Collaborator

@cal101 cal101 left a comment

Choose a reason for hiding this comment

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

2 bugs, see comments.

if (!actualName.contains(File.separator)){
return null;
}
return getActualName().substring(0, getActualName().lastIndexOf("/") + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

internal name is "/" based. File.separator is wrong here.

}

public String getSimpleName() {
int index = getActualName().lastIndexOf(File.separator);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be '/'


public String getCanonicalName() {
return getPackage() + "." + getSimpleName().replaceAll("$", "\\.");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Must be "replace"

Copy link
Owner Author

Choose a reason for hiding this comment

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

I do not agree. I may have multiple nesting levels, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We had the same discussion 2 (?) years ago. :-)
"replace(a,b)" replaces all strings "a" with string "b".
replaceAll(regular-expression, replacement) replaces all occurences of the regular expression. And the regexp "$" means "end of line".

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oops 😄

rpau and others added 7 commits April 17, 2018 18:00
This patch replaces reflection by ASM and increases
the performace almost a 20%. ASM parses binary files and
allows to extract the minimum amount of information that
is required to analyze.
This patch replaces reflection by ASM and increases
the performace almost a 20%. ASM parses binary files and
allows to extract the minimum amount of information that
is required to analyze.
@rpau rpau changed the base branch from rpau-index to master April 17, 2018 17:00
@rpau
Copy link
Owner Author

rpau commented Apr 17, 2018

requests addressed

@rpau
Copy link
Owner Author

rpau commented Apr 25, 2018

ping @cal101

@cal101
Copy link
Collaborator

cal101 commented Oct 27, 2018

Moin Raquel!

How are you!
I am not sure what you want me to do?
There is atleast wrong usage of File.separator in asmclass which will fail only on non unix but nevertheless ...
Style or separation issues can be put aside.

@octopatch
Copy link
Collaborator

octopatch commented Mar 17, 2019

@rpau remember that you have a pending review request

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.

4 participants