Community
Participate
Working Groups
The MethodParameters attribute must be recognized and correctly read by a class file reader in order to properly implement the Java SE platform class libraries (§2.12), if the class file's version number is 52.0 or above and the Java virtual machine implementation recognizes class files whose version number is 52.0 or above.
Created attachment 232767 [details] Patch for this issue This patch supports the parsing of MethodParameters for classfile reading and disassembly
Could someone review this before I commit?
Stephan is probably a better person to review this, if he can spare some time i.e. Otherwise, I will take a look at the patch.
I'll take a look.
Thanks, Stephan -- Note: I didn't implement this for the annotation processor yet.
Some questions, nit-picking & a bug: Re classfmt.MethodInfo: (1) MethodInfo.decodeMethodParameters(): what's the parameter "b" for? (2) I didn't fully understand the handling of "absent" parameter names. decoding may skip a parameter (if nameIndex == 0), but the tests don't exercise this path. While this may just be defensive programming, wouldn't that break assumptions about correspondence of indices? (3) This class skips any access_flags. Are those irrelevant for JDT? Maybe the flags would come in handy to decide which names to expose? (4) Is the relation to a local variables attribute specified somewhere? Given that in the implementation both attributes share the same field, couldn't it happen, that information if overwritten / lost? (5) Why is MethodInfo.argumentNamesIndex a field (vs. local variable)? IModifierConstants: (6) javadoc of ACC_MANDATED looks incomplete / truncated Plus: what's the official spelling: Java 8 or Java SE 8 ? IMethodParametersAttribute: (7) class javadoc: "Description of a methods attribute" isn't quite right. (8) javadoc of getMethodParameterLength() is bogus (copy&paste) (9) wouldn't "the parameter name for the i'th element" be better phrased as "name of the i'th parameter"? If changed pls sync with getAccessFlags(). MethodParametersAttribute: (10) Inside the constructor computation of 'mask' is bogus - int mask = u2At(classFileBytes, 2, readOffset+2); + int mask = u2At(classFileBytes, 2, readOffset); I guess once you have an implementation for creating this attribute, you should come back and write more tests also for decoding.
(In reply to comment #6) > Some questions, nit-picking & a bug: Thank you! > Re classfmt.MethodInfo: > > (1) MethodInfo.decodeMethodParameters(): what's the parameter "b" for? Copy-waste. > (2) I didn't fully understand the handling of "absent" parameter names. > decoding may skip a parameter (if nameIndex == 0), but the tests don't > exercise this path. While this may just be defensive programming, wouldn't that > break assumptions about correspondence of indices? Yep, fixed that. Such unnamed parameters are used for mandated arguments. I'm changing this to be 'arg0' and so forth. > (3) This class skips any access_flags. Are those irrelevant for JDT? > Maybe the flags would come in handy to decide which names to expose? The interesting bits are MANDATED, used for outer this referenes, and SYNTHETIC, used for escaping outer finals in local and anonymous classes. I'm certain the compiler already knows how to deal with these, to the extent that they ever need to be parsed. > (4) Is the relation to a local variables attribute specified somewhere? > Given that in the implementation both attributes share the same field, > couldn't it happen, that information if overwritten / lost? This is intentional. The 'decodeLocalVariableAttribute' is only ever called to decode the parameter names, since they're available together with the other locals with the code if compiled with -g. Local variable names aren't stored, note how it compares the startingPC with 0 and only keeps those. If the MethodParameters block is present, it should take precedence. > (5) Why is MethodInfo.argumentNamesIndex a field (vs. local variable)? This was how it was, but it fixed now. > IModifierConstants: > > (6) javadoc of ACC_MANDATED looks incomplete / truncated > Plus: what's the official spelling: Java 8 or Java SE 8 ? Fixed, and adjusted to Java SE 8 > IMethodParametersAttribute: > > (7) class javadoc: "Description of a methods attribute" isn't quite right. Fixed > (8) javadoc of getMethodParameterLength() is bogus (copy&paste) Fixed > (9) wouldn't "the parameter name for the i'th element" be better phrased as > "name of the i'th parameter"? If changed pls sync with getAccessFlags(). Fixed > MethodParametersAttribute: > > (10) Inside the constructor computation of 'mask' is bogus > - int mask = u2At(classFileBytes, 2, readOffset+2); > + int mask = u2At(classFileBytes, 2, readOffset); Thank you - adding a test case which disassembles these flags PROPERLY > I guess once you have an implementation for creating this attribute, you should > come back and write more tests also for decoding. True - I had some more lined up already, but javac had a bug in the -parameters generation. I've updated and added those. Unfortunately, I can't make javac prepare "missing" parameter names, so that must wait until we can generate them, too. Or I could tweak the .class file, I guess. Should I have another review, or just go ahead and push? I canø
Created attachment 233059 [details] Updated patch following Stephans review Updated patch in git format (has binary contents)
I forgot to add the test case to the relevant suite, kudos to http://www.codeaffine.com/2013/07/04/an-automated-osgi-test-runner/ for reminding me. I wonder if we could do this smarter now we're on CBI/Tycho anyways?
(In reply to comment #8) > Created attachment 233059 [details] > Updated patch following Stephans review > > Updated patch in git format (has binary contents) Stephan, Did you want to glance through the revised patch before +ing the review request ? Thanks.
(In reply to comment #10) > (In reply to comment #8) > > Created attachment 233059 [details] > > Updated patch following Stephans review > > > > Updated patch in git format (has binary contents) > > Stephan, Did you want to glance through the revised patch before +ing the > review request ? Thanks. I'll take a look shortly.
Changes look good to me. Remaining questions / comments: (In reply to comment #7) > > (3) This class skips any access_flags. Are those irrelevant for JDT? > > Maybe the flags would come in handy to decide which names to expose? > > The interesting bits are MANDATED, used for outer this referenes, and SYNTHETIC, > used for escaping outer finals in local and anonymous classes. I'm certain the > compiler already knows how to deal with these, to the extent that they ever need > to be parsed. Can you share why you're certain? If this is about retrieving names from class files, the compiler would normally not bother at all. But when passing information downstream, would it, e.g., be interesting to name some parameters "this$0"... and others "val$something" even if no name was found? Put differently, if the compiler doesn't care about those flags, who should? Are those completely useless? > > (4) Is the relation to a local variables attribute specified somewhere? > > Given that in the implementation both attributes share the same field, > > couldn't it happen, that information if overwritten / lost? > > This is intentional. The 'decodeLocalVariableAttribute' is only ever called to > decode the parameter names, since they're available together with the other > locals with the code if compiled with -g. > Local variable names aren't stored, note how it compares the startingPC with 0 > and only keeps those. This didn't quite convince me, why it's a good idea to share the same field. Thus I had to dig a bit deeper, documenting my findings here: - during createMethod() only decodeMethodParameters() is called, not decodeLocalVariableAttribute() - the latter is called on-demand with only one public entry point: getArgumentNames() - the latter has a check if argument names are already present, so MethodParameters has precedence over LocalVariables. Ergo: I agree that sharing this field is OK, as long as the above chain is maintained. > If the MethodParameters block is present, it should take precedence. s/should/does/ :) > > IMethodParametersAttribute: > > > > (7) class javadoc: "Description of a methods attribute" isn't quite right. > > Fixed I believe "method's parameters names" should be "method's parameter names". (unless you want to say "method's parameters' names"). > I canø ? :) I'll leave the above issues to your judgment, so feel free to release when you feel ready.
Released to BETA_JAVA8 branch.
(In reply to comment #13) > Released to BETA_JAVA8 branch. Thanks, for posterity here is the commit id: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=4ddddb5424dafb4b8650d4349863e03fece6ac06
I reviewed the implementation so as to under the overall picture for JEP118. Looks good. Thanks Jesper. Minor follow up commit here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=adbbf7460be9edd65891c81d46250a9d2d89f5be
