On Wed, 30 May 2007, Conor MacNeill wrote:
> 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
Okay, committed to trunk in r25197.
- application/pgp-signature attachment: stored
Received on Wed May 30 01:46:54 2007