On 30/05/07, Daniel Rall <firstname.lastname@example.org> wrote:
> A few issues:
> - You continue to use the "if (loadException == null)" check after the
> first two attempts to load the library. These will never actually
> succeed in setting "loadException" though, will they? (Just as the
> second won't succeed if the "subversion.native.library" system
> property is set.)
- You unrolled the exception handling from the nested try/catch blocks
> which were previously used. While not necessarily a bad change, this
> formatting change makes the diff harder to read and review.
I thought it was clearer not to keep going down nested try/catch especially
as the throw out of the most nested catch was not really obvious. I think
the for loop approach in your version is better anyway.
- You used tabs instead of spaces, which makes the diff more difficult
> to review, and don't conform to the Subversion project's coding
Sorry about that - don't normally use Visual Studio for Java editing and
only set it to use spaces half way through making the change. Will be more
careful in future.
I'm attaching a revised patch which attempts to address the second two
> items. I'm not sure the first is worth addressing. Thoughts?
+1 from me
Received on Wed May 30 01:15:50 2007