On 31 October 2013 12:05, Mark Thomas <ma...@apache.org> wrote: > On 30/10/2013 21:36, sebb wrote: >> On 30 October 2013 19:31, Mark Thomas <ma...@apache.org> wrote: >>> On 30/10/2013 17:52, Mark Thomas wrote: >>> >>> <snip/> >>> >>>> Please review the release candidate and vote. >>>> This vote will close no sooner that 72 hours from now >>>> >>>> [ ] +1 Release these artifacts >>>> [ ] +0 OK, but... >>>> [ ] -0 OK, but really should fix... >>>> [X] -1 I oppose this release because... >>> >>> I've just changed the API to break a cyclic dependency. There is >>> definitely going to need to be another RC. I'll leave this vote open for >>> now to give folks a chance to find any other issues. >> >> The release notes mention major changes to the API, but don't mention >> that the package name and Maven coordinates have changed from the >> previous pool release. > > Added.
Thanks. Might perhaps be worth mentioning the previous package name and Maven coords? > The RN should be clear that the new features, >> bug fixes etc relate to version 1.? of the code (whatever that is). > > Added. > >> Also, there are a lot of instances of variable hiding, mainly caused >> by local copies with the same name. >> The ones I have checked seem to be harmless, but it may not always be >> clear whether the code should be using the local copy or the hidden >> copy. > > If there is a local copy, the code should be using it. The name clashes > aren't something that particularly bothers me. I see you have fixed them > - that works for me. OK. It's not unknown for name hiding to cause bugs, which is why I think it's worth fixing. A while back I found some code that accidentally used a local variable when it should have updated the class field. >> Also, the ones in LinkedBlockingDeque are completely unnecessary, for >> example: > > The code was copied from Harmony and deliberately only changed where > necessary to expose the additional information required by pool. Not > making other changes when copying code like this is a habit I've picked > up to make it easy to sync future changes to the code. Given that future > changes are unlikely to Harmony, I have no objection to you cleaning it > up if you want to scratch that particular itch. OK, done. >> Given that this is the first release of a new package/Maven coords, it >> would be sensible to ensure that any internal classes are clearly >> marked as such, as that would allow them to be changed without >> breaking the public API. > > This should already be the case with internal use only classes made > package private. I found a few errors which I have fixed. Did you have > anything specific in mind? It was a general remark, I had no specific classes in mind. BTW, as you will have seen, I updated the pom so the download page can be created automatically in future. > Mark > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
