-
Notifications
You must be signed in to change notification settings - Fork 124
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 for type.getType(...) use on non-signature type names #221
Conversation
This fix seems to solve problem. It works on my machine. |
@JuditKnoll |
@garydgregory Any status on this fix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazendaz @nbauma109
See my comment.
@@ -365,6 +366,24 @@ public int hashCode() { | |||
return type ^ signature.hashCode(); | |||
} | |||
|
|||
static String internalTypeNameToSignature(final String internalTypeName) { | |||
if (StringUtils.isEmpty(internalTypeName) || StringUtils.equalsAny(internalTypeName, "B", "C", "D", "F", "I", "J", "S", "Z")) { | |||
return internalTypeName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not crazy about this magic list of strings. Could we reuse or refactor org.apache.bcel.generic.InstructionFactory.SHORT_NAMES
(by making it public)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array has different values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any array containing these exact short names. The mentioned SHORT_NAMES misses Z
, and J
, but contains L
additionally. The SHORT_TYPE_NAMES
in Consts and Constants both contain V
and ILLEGAL_TYPE
unnecessarily. There are constants for the short names individually in ElementValue, but I'm not sure using those would help a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi all,
I'm looking for clarity when reading the code. A hard coded array of strings is bad. Having a good name for a constant would help, and SHORT_NAMES is not a good name when the class is Constants or Consts. Any improvement is appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since none of the existing arrays could be reused here because of the different values, the renaming should be in a separate PR, not in this one. In the JVM specification these short names are introduced as field descriptors, but that name may cause some confusion with the field and it rather refers to the type of the fields.
As far as I can see, this particular "array" contains the primitive or base types, but naming it is the privilege of the author.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstructionFactory.SHORT_NAMES wasn't a good fit but Const.SHORT_TYPE_NAMES looks better suited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garydgregory is it ok now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garydgregory any chance you can have a look at it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometime this week...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garydgregory can you please look at this PR?
@@ -180,7 +181,7 @@ public static String getSignature(final java.lang.reflect.Method meth) { | |||
public static Type getType(final Class<?> cls) { | |||
Objects.requireNonNull(cls, "cls"); | |||
/* | |||
* That's an amzingly easy case, because getName() returns the signature. That's what we would have liked anyway. | |||
* That's an amazingly easy case, because getName() returns the signature. That's what we would have liked anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one! :-)
Codecov Report
@@ Coverage Diff @@
## master #221 +/- ##
============================================
+ Coverage 64.73% 64.75% +0.02%
- Complexity 3872 3878 +6
============================================
Files 364 364
Lines 15666 15675 +9
Branches 1943 1946 +3
============================================
+ Hits 10142 10151 +9
Misses 4602 4602
Partials 922 922
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@garydgregory thanks 👍 |
This is a fix for a small regression introduced by PR #171 with the use of Type.getType(...) on an internal type name which needs to be converted to a signature first (i.e. java/lang/String => Ljava/lang/String;)