Thanks for taking the time to review those patches Mark. I guarantee you that Mark and Jerome do not know that their patches are considered difficult to review in a timely manner. The last comment on all of their patches is from them and they're waiting for a response. If the issues are considered not clear enough and require more explanation, it would be helpful to let them and the community know that instead of letting the issues languish. They seem very willing to do some work to get their patches in. Let them know what an ideal to review patch looks like and how there's still a lot of burden on the reviewer in the ones they've submitted.
Mark, Jerome, perhaps you could review each others patches? If you agree to review each others patches, it could help your patches get in much more quickly. I know it's not fair I ask you to do more work without doing it myself, but I'm nowhere near familiar enough with the codebase to be of any help. Perhaps I could offer my assistance in some other way? I can run tests on various JVMs, write Java code that doesn't require such complex knowedge of bytecode, or make a monetary donation to the Apache Foundation if any of those things would help. Doing what I can, I just took some time to look at 183 <https://issues.apache.org/jira/browse/BCEL-183>. I guess you're right that the patch could be split between one patch for field name and one patch for method name if that seems helpful. Would you want me to open new issues with the patch split in two for that? Should we check with Jerome and Emmanuel before doing that? What is it that you're thinking we could do in terms of a common test framework? I don't see a lot of code duplication or anything. --S. On Fri, Mar 6, 2015 at 3:19 PM, Mark Thomas <ma...@apache.org> wrote: > On 06/03/2015 21:05, Sarah Murray wrote: > > - 187 <https://issues.apache.org/jira/browse/BCEL-187> - has a test > > case. no committer review for 2 months > > Neither has anyone else reviewed it. It isn't just committers that can > review patches. I've got the necessary commit karma and after looking at > that issue for a minute or two I can see a whole pile of work that needs > to be done before I'd even think about committing anything. Just from > that quick look: > 1. Review the svn history to see if there are any pointers to which part > of the JVM spec defines the restriction. > 2. Review the JVM spec to see if I can see the restriction > 3. Look at the byte code for the provided example to see which class the > invoke opcode references. Better yet, write a disabled BCEL unit test to > show this. > 4. The structure of the test case looks odd. I suspect it aligns with > other patches for other issues but nowhere is that explained. > 5. The class files provided do not have any attached source. Without the > source code, we have no idea (OK I could fire up a decompiler but that > is another task) what is actually in those classes. > > To put it another way, the current code asserts one behaviour. The bug > report asserts another. No evidence is provided to support either > position. Given that most (all?) the current committers likely to work > on BCEL don't have a deep knowledge of the JVM specs some research is > required to figure out which is correct. I've set out above how I'd do > the research. I'm sure there are other, better approaches. Doing the > research doesn't require commit karma. > > > - 188 <https://issues.apache.org/jira/browse/BCEL-188> - has a test > > case. no committer review for 2 months > > Looks to be in better shape than 187 but still no source provided for > one of the test classes. > > > - 183 <https://issues.apache.org/jira/browse/BCEL-183> - has a test > > case. no committer review for 3 months > > It is not at all clear how much of the original issue (if any) was fixed > by the change reported 19-Dec-2014. > > The potential for the required behaviour to vary with class file version > is a complicating factor. > > This issues seems to be bundling up a number of smaller issues. That > makes it harder to work with even if they are all in the same area. > > Generally for all of these issues some pointers to the relevant parts of > the JVM spec - preferably as comments in the patch - would make it > easier for folks to review the patches and easier for future maintainers > to understand what the code is doing. > > Also generally, it looks like a number of the patches could be reduced > in size by pulling out the common testing framework. > > > > The problem is that there's one area where help is really lacking and the > > community can't contribute there because not everyone has the commit > access > > that's needed to get things flowing again. > > I disagree with that analysis. There is plenty that could be done to > move these issues forwards without commit access. I can find the odd > hour here and there to look at BCEL issues but looking at the three > issues you quoted above all of them would require far more than an hours > work to get to a position where I'd be happy to commit something. Pretty > much all of that work could be done by anyone on this list. > > > Perhaps we could add more committers since it's currently a bottleneck? > > Committers will be added as and when folks demonstrate the appropriate > merit. > > > E.g. Mark and Jerome both have submitted numerous high quality patches > and > > seem to be well-regarded members of the community. Can we give them > commit > > access? > > I agree they look to be heading in the right direction. I'm not familiar > enough with the BCEL code base to judge the quality of the patches in > their current form. > > > If we're nervous about maintaining high code quality we can add > > guidelines such as there must be a test case for any commit and you can't > > commit your own code - someone else needs to review it. > > You really don't want to introduce those sorts of rules if you can help > it. It would slow progress even further. > > > There was talk of an alpha release and it seems like folks are in favor > of > > it. Is there anything I can help to do with regards to that? > > Right now the most useful thing folks can do is pick a bug that matters > to them and then work with the community to get the issue to the point > where a current committer can resolve it with no more than 60 minutes of > effort. > > I can find 60 minutes over the next few days. Which issue do you want to > fix first? > > I'm also more than happy to commit common test code to reduce the > size/complexity of any proposed patches. > > Mark > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
