Daniel Rall wrote:
> On Tue, 29 May 2007, Conor MacNeill wrote:
>
>> This change retains the first thrown exception when attempting to load
>> the svnjavahl-1 native library rather than throwing the exception from
>> the last attempt. This is important when the user specifies the library
>> to load and it cannot be loaded. At present the error that finally gets
>> returned to the user is not the one related to the attempt to load their
>> library.
>>
>> On my system for example, the change lets me see this error:
>>
>> Could not load svn-javahl:
>> java.lang.UnsatisfiedLinkError-C:\Software\svn-dev\javahl\libsvnjavahl-1.dll:
>> Can't load IA 32-bit .dll on a AMD 64-bit platform
>
> Sounds like a good idea, Connor.
>
>> which before I could not. Here is my suggested log message:
>>
>> [[[
>> Better error reporting when unable to load the Native JavaHL library
>>
>> *
>> subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java
>> (loadNativeLibrary): Use the earliest exception to report to the user
>> rather than the last
>> ]]]
>>
>> Let me know if I goofed.
>
> 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.
>
> - 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.
>
> I'm attaching a revised patch which attempts to address the second two
> items. I'm not sure the first is worth addressing. Thoughts?
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java
> ===================================================================
> --- subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java (revision 25190)
> +++ subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java (working copy)
> @@ -47,6 +47,8 @@
> */
> public static synchronized void loadNativeLibrary()
> {
> + UnsatisfiedLinkError loadException = null;
> +
> // If the user specified the fully qualified path to the
> // native library, try loading that first.
> try
> @@ -62,32 +64,39 @@
> }
> catch (UnsatisfiedLinkError ex)
> {
> - // ignore that error to try again
> + // Since the user explicitly specified this path, this is
> + // the best error to return if no other method succeeds.
> + loadException = ex;
> }
>
> // Try to load the library by its name. Failing that, try to
> // load it by its old name.
> - try
> + String[] libraryNames = {"svnjavahl-1", "libsvnjavahl-1", "svnjavahl"};
> + for (int i = 0; i < libraryNames.length; i++)
> {
> - System.loadLibrary("svnjavahl-1");
> - init();
> - return;
> - }
> - catch (UnsatisfiedLinkError ex)
> - {
> try
> {
> - System.loadLibrary("libsvnjavahl-1");
> + System.loadLibrary(libraryNames[i]);
> init();
> return;
> }
> - catch (UnsatisfiedLinkError e)
> + catch (UnsatisfiedLinkError ex)
> {
> - System.loadLibrary("svnjavahl");
> - init();
> - return;
> + if (loadException == null)
> + {
> + loadException = ex;
> + }
> }
> }
> +
> + // Re-throw the most relevant exception.
> + if (loadException == null)
> + {
> + // This could only happen as the result of a programming error.
> + loadException = new UnsatisfiedLinkError("Unable to load JavaHL " +
> + "native library");
> + }
> + throw loadException;
> }
>
> /**
I like the idea of the patch.
How about using
if (null == loadException)
instead of
if (loadException == null)
This style of coding prevents the accidental assignment of null to
loadException.
Regards,
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 30 00:37:12 2007