Re: [PATCH] Better reporting of JavaHL loading exceptions

Daniel,

On 30/05/07, Daniel Rall <dlr@collab.net> 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.)

true

- 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
> conventions.

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

Conor
Received on Wed May 30 01:15:50 2007