Updated webrev here: http://cr.openjdk.java.net/~emc/8020981/
Are there any more comments, or is this good to go? On 09/19/13 18:15, Eric McCorkle wrote: > The webrev has been updated with Joe's comments addressed. > > On 09/19/13 00:11, David Holmes wrote: >> On 19/09/2013 9:59 AM, Eric McCorkle wrote: >>> This still needs to be reviewed. >> >> You seem to have ignored Joe's comments regarding the change to Modifier >> being incorrect. >> >> David >> >>> On 09/16/13 14:50, Eric McCorkle wrote: >>>> I pulled the class files into byte array constants, as a temporary >>>> measure until a viable method for testing bad class files is developed. >>>> >>>> The webrev has been refreshed. The class files will be taken out before >>>> I push. >>>> >>>> http://cr.openjdk.java.net/~emc/8020981/ >>>> >>>> On 09/13/13 20:48, Joe Darcy wrote: >>>>> To avoid storing binaries in Hg, you could try something like: >>>>> >>>>> * uuencode / ascii armor the class file >>>>> * convert to byte array in the test >>>>> * load classes from byte array >>>>> >>>>> -Joe >>>>> >>>>> On 09/13/2013 11:54 AM, Eric McCorkle wrote: >>>>>> I did it by hand with emacs. >>>>>> >>>>>> I would really rather tackle the bad class files for testing issue >>>>>> once >>>>>> and for all, the Right Way (tm). But with ZBB looming, now is not the >>>>>> time to do it. >>>>>> >>>>>> Hence, I have created this task >>>>>> https://bugs.openjdk.java.net/browse/JDK-8024674 >>>>>> >>>>>> I also just created this one: >>>>>> https://bugs.openjdk.java.net/browse/JDK-8024812 >>>>>> >>>>>> On 09/13/13 13:54, Peter Levart wrote: >>>>>>> Hi Eric, >>>>>>> >>>>>>> How did you create those class files? By hand using a HEX editor? Did >>>>>>> you create a program that patched the original class file? If the >>>>>>> later >>>>>>> is the case, you could pack that patching logic inside a custom >>>>>>> ClassLoader... >>>>>>> >>>>>>> To hacky? Dependent on future changes of javac? At least the "bad >>>>>>> name" >>>>>>> patching could be performed trivially and reliably, I think: >>>>>>> search and >>>>>>> replace with same-length string. >>>>>>> >>>>>>> Regards, Peter >>>>>>> >>>>>>> On 09/13/2013 07:35 PM, Eric McCorkle wrote: >>>>>>>> Ugh. Byte arrays of class file data is really a horrible solution. >>>>>>>> >>>>>>>> I have already filed a task for test development post ZBB to >>>>>>>> develop a >>>>>>>> solution for generating bad class files. I'd prefer to file a >>>>>>>> follow-up >>>>>>>> to this to add the bad class file tests when that's done. >>>>>>>> >>>>>>>> On 09/13/13 10:55, Joel Borggrén-Franck wrote: >>>>>>>>> I think the right thing to do is to include the original compiling >>>>>>>>> source in a comment, together with a comment on how you modify >>>>>>>>> them, and then the result as a byte array. >>>>>>>>> >>>>>>>>> IIRC I have seen test of that kind before somewhere in our repo. >>>>>>>>> >>>>>>>>> cheers >>>>>>>>> /Joel >>>>>>>>> >>>>>>>>> On Sep 13, 2013, at 4:49 PM, Eric McCorkle >>>>>>>>> <eric.mccor...@oracle.com> wrote: >>>>>>>>> >>>>>>>>>> There is no simple means of generating bad class files for >>>>>>>>>> testing. >>>>>>>>>> This is a huge deficiency in our testing abilities. >>>>>>>>>> >>>>>>>>>> If these class files shouldn't go in, then I'm left with no choice >>>>>>>>>> but >>>>>>>>>> to check in no test for this patch. >>>>>>>>>> >>>>>>>>>> However, anyone can run the test I've provided with the class >>>>>>>>>> files and >>>>>>>>>> see that it works. >>>>>>>>>> >>>>>>>>>> On 09/13/13 09:55, Joel Borggrén-Franck wrote: >>>>>>>>>>> Hi Eric, >>>>>>>>>>> >>>>>>>>>>> IIRC we don't check in classfiles into the repo. >>>>>>>>>>> >>>>>>>>>>> I'm not sure how we handle testing of broken class-files in jdk, >>>>>>>>>>> but ASM might be an option, or storing the class file as an >>>>>>>>>>> embedded byte array in the test. >>>>>>>>>>> >>>>>>>>>>> cheers >>>>>>>>>>> /Joel >>>>>>>>>>> >>>>>>>>>>> On Sep 13, 2013, at 3:40 PM, Eric McCorkle >>>>>>>>>>> <eric.mccor...@oracle.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> A new webrev is posted (and crucible updated), which actually >>>>>>>>>>>> validates >>>>>>>>>>>> parameter names correctly. Apologies for the last one. >>>>>>>>>>>> >>>>>>>>>>>> On 09/12/13 16:02, Eric McCorkle wrote: >>>>>>>>>>>>> Hello, >>>>>>>>>>>>> >>>>>>>>>>>>> Please review this patch, which implements correct behavior for >>>>>>>>>>>>> the >>>>>>>>>>>>> Parameter Reflection API in the case of malformed class files. >>>>>>>>>>>>> >>>>>>>>>>>>> The bug report is here: >>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8020981 >>>>>>>>>>>>> >>>>>>>>>>>>> The webrev is here: >>>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8020981/ >>>>>>>>>>>>> >>>>>>>>>>>>> This review is also on crucible. The ID is CR-JDK8TL-182. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> Eric >>>>>>>>>>>>> >>>>>>>>>> <eric_mccorkle.vcf> >>>>>
