You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2007/04/26 16:39:18 UTC
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
On Thu, 26 Apr 2007, hwright@tigris.org wrote:
...
> JavaHL: Eliminate duplicate native implementations of propertyCreate(), and
> implement one version as a wrapper around the other. Also, modify the
> properties test to use propertyCreate() as well as propertySet().
...
> --- trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp (original)
> +++ trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp Thu Apr 26 09:00:39 2007
...
> -JNIEXPORT void JNICALL
> -Java_org_tigris_subversion_javahl_SVNClient_propertyCreate__Ljava_lang_String_2Ljava_lang_String_2_3BZZ
> -(JNIEnv *env, jobject jthis, jstring jpath, jstring jname, jbyteArray jvalue,
> - jboolean jrecurse, jboolean jforce)
> -{
> - JNIEntry(SVNClient, propertyCreate);
> - SVNClient *cl = SVNClient::getCppObject(jthis);
> - if (cl == NULL)
> - {
> - JNIUtil::throwError(_("bad C++ this"));
> - return;
> - }
> - JNIStringHolder path(jpath);
> - if (JNIUtil::isExceptionThrown())
> - return;
> -
> - JNIStringHolder name(jname);
> - if (JNIUtil::isExceptionThrown())
> - return;
> -
> - JNIByteArray value(jvalue);
> - if (JNIUtil::isExceptionThrown())
> - return;
> -
> - cl->propertyCreate(path, name, (const char *)value.getBytes(),
> - jrecurse ? true:false,
> - jforce ? true:false);
> -}
> -
> JNIEXPORT jobject JNICALL
> Java_org_tigris_subversion_javahl_SVNClient_revProperty
> (JNIEnv *env, jobject jthis, jstring jpath, jstring jname, jobject jrevision)
>
> Modified: trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java?pathrev=24791&r1=24790&r2=24791
> ==============================================================================
> --- trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (original)
> +++ trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java Thu Apr 26 09:00:39 2007
> @@ -1447,9 +1447,12 @@
> * @throws ClientException
> * @since 1.2
> */
> - public native void propertyCreate(String path, String name, byte[] value,
> - boolean recurse, boolean force)
> - throws ClientException;
> + public void propertyCreate(String path, String name, byte[] value,
> + boolean recurse, boolean force)
> + throws ClientException
> + {
> + propertyCreate(path, name, new String(value), recurse, force);
> + }
...
Isn't "new String(value)" doing an implicit byte -> char conversion in
the JVM's default character set? Could this be lossy?
It might be safer to make this change the other way around...
byte -> char conversions in property setting code
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 01 May 2007, Hyrum K. Wright wrote:
> Blair Zajac wrote:
...
> >>>> While I'm not absolutely sure what the Right Thing is here, getting
> >>>> the byte[] representation of a Java String using the JVM's default
> >>>> encoding (which unfortunately might not be UTF-8), then basically
> >>>> transforming each byte in that array into a character, is definitely
> >>>> not correct.
> >>>
> >>> To clarify: That's what we used to do.
> >>>
> >>> Now we seem to do the opposite. However, if we don't know what
> >>> encoding to create a String from using those bytes, using the JVM's
> >>> default encoding may not be correct, either.
> >>
> >> Can't we use
...
> > value.getBytes( "UTF-8" );
>
> The attached patch uses a byte array instead of a String to move the
> property values from the Java layer to the C++ layer. I have a little
> misgiving about using a ClientException, but I don't see a better way to
> implement the error handling when specifying the UTF-8 encoding.
--- subversion/bindings/javahl/native/SVNClient.cpp (revision 24864)
+++ subversion/bindings/javahl/native/SVNClient.cpp (working copy)
...
void SVNClient::propertySet(const char *path, const char *name,
- const char *value, bool recurse, bool force)
+ JNIByteArray &value, bool recurse, bool force)
{
Pool requestPool;
SVN_JNI_NULL_PTR_EX(path, "path", );
SVN_JNI_NULL_PTR_EX(name, "name", );
- SVN_JNI_NULL_PTR_EX(value, "value", );
- svn_string_t *val = svn_string_create(value, requestPool.pool());
+ if (value.isNull())
+ {
+ JNIUtil::throwNullPointerException("value");
+ return;
+ }
+ svn_string_t *val = svn_string_ncreate((const char *)value.getBytes(),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Is this really kosher? An element from an array of bytes from a UTF-8
string may or may not correspond to a UTF-8 character. This is
similar to what we used to be doing -- does it "just work" for some
reason?
+ value.getLength(),
+ requestPool.pool());
propertySet(path, name, val, recurse, force, SVN_INVALID_REVNUM);
}
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by Mark Phippard <ma...@gmail.com>.
On 5/1/07, Daniel Rall <dl...@collab.net> wrote:
> I am unconvinced that the previous code actually worked for multi-byte
> character encodings.
That is actually what I figured, but no one had specifically said it until now.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 01 May 2007, Mark Phippard wrote:
> On 5/1/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
> >
> >The attached patch uses a byte array instead of a String to move the
> >property values from the Java layer to the C++ layer. I have a little
> >misgiving about using a ClientException, but I don't see a better way to
> >implement the error handling when specifying the UTF-8 encoding.
>
> I have only loosely followed this thread and do not really understand
> the C++ code enough to comment. But stepping back, we had two methods
> that presumably worked and did roughly the same thing. We wanted to
> apparently consolidate the C++ code to a single method. If both of
> the previous methods worked correctly than what is the complication
> here in terms of the encoding?
>
> In general I am just concerned when we are taking working code and
> changing it to eliminate redundancy but are not 100% sure we are not
> now adding problems.
I am unconvinced that the previous code actually worked for multi-byte
character encodings.
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by Mark Phippard <ma...@gmail.com>.
On 5/1/07, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
>
> The attached patch uses a byte array instead of a String to move the
> property values from the Java layer to the C++ layer. I have a little
> misgiving about using a ClientException, but I don't see a better way to
> implement the error handling when specifying the UTF-8 encoding.
I have only loosely followed this thread and do not really understand
the C++ code enough to comment. But stepping back, we had two methods
that presumably worked and did roughly the same thing. We wanted to
apparently consolidate the C++ code to a single method. If both of
the previous methods worked correctly than what is the complication
here in terms of the encoding?
In general I am just concerned when we are taking working code and
changing it to eliminate redundancy but are not 100% sure we are not
now adding problems.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native
src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Blair Zajac wrote:
>
> On Apr 26, 2007, at 7:07 PM, Blair Zajac wrote:
>
>>
>> On Apr 26, 2007, at 6:04 PM, Daniel Rall wrote:
>>
>>> On Thu, 26 Apr 2007, Daniel Rall wrote:
>>>
>>>> On Thu, 26 Apr 2007, Hyrum K. Wright wrote:
>>>>
>>>>> Daniel Rall wrote:
>>>>>> On Thu, 26 Apr 2007, hwright@tigris.org wrote:
>>>>>> ...
>>>>>>> JavaHL: Eliminate duplicate native implementations of
>>>>>>> propertyCreate(), and
>>>>>>> implement one version as a wrapper around the other. Also,
>>>>>>> modify the
>>>>>>> properties test to use propertyCreate() as well as propertySet().
>>>>>> ...
>>>>>>> ---
>>>>>>> trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp
>>>>>>> (original)
>>>>>>> +++
>>>>>>> trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp
>>>>>>> Thu Apr 26 09:00:39 2007
>>>> ...
>>>>>>> - JNIByteArray value(jvalue);
>>>>>>> - if (JNIUtil::isExceptionThrown())
>>>>>>> - return;
>>>>>>> -
>>>>>>> - cl->propertyCreate(path, name, (const char *)value.getBytes(),
>>>>>>> - jrecurse ? true:false,
>>>>>>> - jforce ? true:false);
>>>>>>> -}
>>>> ...
>>>>>>> ---
>>>>>>> trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java
>>>>>>> (original)
>>>>>>> +++
>>>>>>> trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java
>>>>>>> Thu Apr 26 09:00:39 2007
>>>>>>> @@ -1447,9 +1447,12 @@
>>>>>>> * @throws ClientException
>>>>>>> * @since 1.2
>>>>>>> */
>>>>>>> - public native void propertyCreate(String path, String name,
>>>>>>> byte[] value,
>>>>>>> - boolean recurse, boolean
>>>>>>> force)
>>>>>>> - throws ClientException;
>>>>>>> + public void propertyCreate(String path, String name, byte[]
>>>>>>> value,
>>>>>>> + boolean recurse, boolean force)
>>>>>>> + throws ClientException
>>>>>>> + {
>>>>>>> + propertyCreate(path, name, new String(value), recurse,
>>>>>>> force);
>>>>>>> + }
>>>>>> ...
>>>>>>
>>>>>> Isn't "new String(value)" doing an implicit byte -> char
>>>>>> conversion in
>>>>>> the JVM's default character set? Could this be lossy?
>>>>>>
>>>>>> It might be safer to make this change the other way around...
>>>>>
>>>>> Sure. The motivation for doing it this way was consistency with a
>>>>> similar construct in propertySet(). I'd be happy to change them
>>>>> both if
>>>>> needed.
>>>>
>>>> While I'm not absolutely sure what the Right Thing is here, getting
>>>> the byte[] representation of a Java String using the JVM's default
>>>> encoding (which unfortunately might not be UTF-8), then basically
>>>> transforming each byte in that array into a character, is definitely
>>>> not correct.
>>>
>>> To clarify: That's what we used to do.
>>>
>>> Now we seem to do the opposite. However, if we don't know what
>>> encoding to create a String from using those bytes, using the JVM's
>>> default encoding may not be correct, either.
>>
>> Can't we use
>>
>> http://java.sun.com/j2se/1.5.0/docs/api/java/lang/String.html#getBytes(java.lang.String)
>>
>
> And use
>
> value.getBytes( "UTF-8" );
>
> ???
The attached patch uses a byte array instead of a String to move the
property values from the Java layer to the C++ layer. I have a little
misgiving about using a ClientException, but I don't see a better way to
implement the error handling when specifying the UTF-8 encoding.
-Hyrum
[[[
JavaHL: Use a byte array instead of a String to pass property values
from the Java layer to the C++ layer.
Sugggested by: dlr
blair
[ in subversion/bindings/javahl ]
* native/org_tigris_subversion_javahl_SVNClient.cpp
(Java_org_tigris_subversion_javahl_SVNClient_propertySet):
Update arguments to use a byte array.
* native/SVNClient.h,
native/SVNClient.cpp
(propertySet): Accept a JNIByteArray in place of a const char * for
the property value.
* src/org/tigris/subversion/javahl/SVNClient.java
(propertySet): Use a native implementation for the byte array version.
(propertySet): Implement the String version as a wrapper around the
byte array version. Convert the String to bytes by using the UTF-8
encoding. Catch a potential encoding exception, and re-throw it as a
ClientException.
]]]
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by Blair Zajac <bl...@orcaware.com>.
On Apr 26, 2007, at 7:07 PM, Blair Zajac wrote:
>
> On Apr 26, 2007, at 6:04 PM, Daniel Rall wrote:
>
>> On Thu, 26 Apr 2007, Daniel Rall wrote:
>>
>>> On Thu, 26 Apr 2007, Hyrum K. Wright wrote:
>>>
>>>> Daniel Rall wrote:
>>>>> On Thu, 26 Apr 2007, hwright@tigris.org wrote:
>>>>> ...
>>>>>> JavaHL: Eliminate duplicate native implementations of
>>>>>> propertyCreate(), and
>>>>>> implement one version as a wrapper around the other. Also,
>>>>>> modify the
>>>>>> properties test to use propertyCreate() as well as propertySet().
>>>>> ...
>>>>>> --- trunk/subversion/bindings/javahl/native/
>>>>>> org_tigris_subversion_javahl_SVNClient.cpp (original)
>>>>>> +++ trunk/subversion/bindings/javahl/native/
>>>>>> org_tigris_subversion_javahl_SVNClient.cpp Thu Apr 26 09:00:39
>>>>>> 2007
>>> ...
>>>>>> - JNIByteArray value(jvalue);
>>>>>> - if (JNIUtil::isExceptionThrown())
>>>>>> - return;
>>>>>> -
>>>>>> - cl->propertyCreate(path, name, (const char *)value.getBytes(),
>>>>>> - jrecurse ? true:false,
>>>>>> - jforce ? true:false);
>>>>>> -}
>>> ...
>>>>>> --- trunk/subversion/bindings/javahl/src/org/tigris/subversion/
>>>>>> javahl/SVNClient.java (original)
>>>>>> +++ trunk/subversion/bindings/javahl/src/org/tigris/subversion/
>>>>>> javahl/SVNClient.java Thu Apr 26 09:00:39 2007
>>>>>> @@ -1447,9 +1447,12 @@
>>>>>> * @throws ClientException
>>>>>> * @since 1.2
>>>>>> */
>>>>>> - public native void propertyCreate(String path, String
>>>>>> name, byte[] value,
>>>>>> - boolean recurse,
>>>>>> boolean force)
>>>>>> - throws ClientException;
>>>>>> + public void propertyCreate(String path, String name, byte
>>>>>> [] value,
>>>>>> + boolean recurse, boolean force)
>>>>>> + throws ClientException
>>>>>> + {
>>>>>> + propertyCreate(path, name, new String(value),
>>>>>> recurse, force);
>>>>>> + }
>>>>> ...
>>>>>
>>>>> Isn't "new String(value)" doing an implicit byte -> char
>>>>> conversion in
>>>>> the JVM's default character set? Could this be lossy?
>>>>>
>>>>> It might be safer to make this change the other way around...
>>>>
>>>> Sure. The motivation for doing it this way was consistency with a
>>>> similar construct in propertySet(). I'd be happy to change them
>>>> both if
>>>> needed.
>>>
>>> While I'm not absolutely sure what the Right Thing is here, getting
>>> the byte[] representation of a Java String using the JVM's default
>>> encoding (which unfortunately might not be UTF-8), then basically
>>> transforming each byte in that array into a character, is definitely
>>> not correct.
>>
>> To clarify: That's what we used to do.
>>
>> Now we seem to do the opposite. However, if we don't know what
>> encoding to create a String from using those bytes, using the JVM's
>> default encoding may not be correct, either.
>
> Can't we use
>
> http://java.sun.com/j2se/1.5.0/docs/api/java/lang/
> String.html#getBytes(java.lang.String)
And use
value.getBytes( "UTF-8" );
???
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by Blair Zajac <bl...@orcaware.com>.
On Apr 26, 2007, at 6:04 PM, Daniel Rall wrote:
> On Thu, 26 Apr 2007, Daniel Rall wrote:
>
>> On Thu, 26 Apr 2007, Hyrum K. Wright wrote:
>>
>>> Daniel Rall wrote:
>>>> On Thu, 26 Apr 2007, hwright@tigris.org wrote:
>>>> ...
>>>>> JavaHL: Eliminate duplicate native implementations of
>>>>> propertyCreate(), and
>>>>> implement one version as a wrapper around the other. Also,
>>>>> modify the
>>>>> properties test to use propertyCreate() as well as propertySet().
>>>> ...
>>>>> --- trunk/subversion/bindings/javahl/native/
>>>>> org_tigris_subversion_javahl_SVNClient.cpp (original)
>>>>> +++ trunk/subversion/bindings/javahl/native/
>>>>> org_tigris_subversion_javahl_SVNClient.cpp Thu Apr 26 09:00:39
>>>>> 2007
>> ...
>>>>> - JNIByteArray value(jvalue);
>>>>> - if (JNIUtil::isExceptionThrown())
>>>>> - return;
>>>>> -
>>>>> - cl->propertyCreate(path, name, (const char *)value.getBytes(),
>>>>> - jrecurse ? true:false,
>>>>> - jforce ? true:false);
>>>>> -}
>> ...
>>>>> --- trunk/subversion/bindings/javahl/src/org/tigris/subversion/
>>>>> javahl/SVNClient.java (original)
>>>>> +++ trunk/subversion/bindings/javahl/src/org/tigris/subversion/
>>>>> javahl/SVNClient.java Thu Apr 26 09:00:39 2007
>>>>> @@ -1447,9 +1447,12 @@
>>>>> * @throws ClientException
>>>>> * @since 1.2
>>>>> */
>>>>> - public native void propertyCreate(String path, String
>>>>> name, byte[] value,
>>>>> - boolean recurse, boolean
>>>>> force)
>>>>> - throws ClientException;
>>>>> + public void propertyCreate(String path, String name, byte
>>>>> [] value,
>>>>> + boolean recurse, boolean force)
>>>>> + throws ClientException
>>>>> + {
>>>>> + propertyCreate(path, name, new String(value), recurse,
>>>>> force);
>>>>> + }
>>>> ...
>>>>
>>>> Isn't "new String(value)" doing an implicit byte -> char
>>>> conversion in
>>>> the JVM's default character set? Could this be lossy?
>>>>
>>>> It might be safer to make this change the other way around...
>>>
>>> Sure. The motivation for doing it this way was consistency with a
>>> similar construct in propertySet(). I'd be happy to change them
>>> both if
>>> needed.
>>
>> While I'm not absolutely sure what the Right Thing is here, getting
>> the byte[] representation of a Java String using the JVM's default
>> encoding (which unfortunately might not be UTF-8), then basically
>> transforming each byte in that array into a character, is definitely
>> not correct.
>
> To clarify: That's what we used to do.
>
> Now we seem to do the opposite. However, if we don't know what
> encoding to create a String from using those bytes, using the JVM's
> default encoding may not be correct, either.
Can't we use
http://java.sun.com/j2se/1.5.0/docs/api/java/lang/String.html#getBytes
(java.lang.String)
Regards,
Blair
--
Blair Zajac, Ph.D.
CTO, OrcaWare Technologies
<bl...@orcaware.com>
Subversion training, consulting and support
http://www.orcaware.com/svn/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 26 Apr 2007, Daniel Rall wrote:
> On Thu, 26 Apr 2007, Hyrum K. Wright wrote:
>
> > Daniel Rall wrote:
> > > On Thu, 26 Apr 2007, hwright@tigris.org wrote:
> > > ...
> > >> JavaHL: Eliminate duplicate native implementations of propertyCreate(), and
> > >> implement one version as a wrapper around the other. Also, modify the
> > >> properties test to use propertyCreate() as well as propertySet().
> > > ...
> > >> --- trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp (original)
> > >> +++ trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp Thu Apr 26 09:00:39 2007
> ...
> > >> - JNIByteArray value(jvalue);
> > >> - if (JNIUtil::isExceptionThrown())
> > >> - return;
> > >> -
> > >> - cl->propertyCreate(path, name, (const char *)value.getBytes(),
> > >> - jrecurse ? true:false,
> > >> - jforce ? true:false);
> > >> -}
> ...
> > >> --- trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (original)
> > >> +++ trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java Thu Apr 26 09:00:39 2007
> > >> @@ -1447,9 +1447,12 @@
> > >> * @throws ClientException
> > >> * @since 1.2
> > >> */
> > >> - public native void propertyCreate(String path, String name, byte[] value,
> > >> - boolean recurse, boolean force)
> > >> - throws ClientException;
> > >> + public void propertyCreate(String path, String name, byte[] value,
> > >> + boolean recurse, boolean force)
> > >> + throws ClientException
> > >> + {
> > >> + propertyCreate(path, name, new String(value), recurse, force);
> > >> + }
> > > ...
> > >
> > > Isn't "new String(value)" doing an implicit byte -> char conversion in
> > > the JVM's default character set? Could this be lossy?
> > >
> > > It might be safer to make this change the other way around...
> >
> > Sure. The motivation for doing it this way was consistency with a
> > similar construct in propertySet(). I'd be happy to change them both if
> > needed.
>
> While I'm not absolutely sure what the Right Thing is here, getting
> the byte[] representation of a Java String using the JVM's default
> encoding (which unfortunately might not be UTF-8), then basically
> transforming each byte in that array into a character, is definitely
> not correct.
To clarify: That's what we used to do.
Now we seem to do the opposite. However, if we don't know what
encoding to create a String from using those bytes, using the JVM's
default encoding may not be correct, either.
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 26 Apr 2007, Hyrum K. Wright wrote:
> Daniel Rall wrote:
> > On Thu, 26 Apr 2007, hwright@tigris.org wrote:
> > ...
> >> JavaHL: Eliminate duplicate native implementations of propertyCreate(), and
> >> implement one version as a wrapper around the other. Also, modify the
> >> properties test to use propertyCreate() as well as propertySet().
> > ...
> >> --- trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp (original)
> >> +++ trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp Thu Apr 26 09:00:39 2007
...
> >> - JNIByteArray value(jvalue);
> >> - if (JNIUtil::isExceptionThrown())
> >> - return;
> >> -
> >> - cl->propertyCreate(path, name, (const char *)value.getBytes(),
> >> - jrecurse ? true:false,
> >> - jforce ? true:false);
> >> -}
...
> >> --- trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (original)
> >> +++ trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java Thu Apr 26 09:00:39 2007
> >> @@ -1447,9 +1447,12 @@
> >> * @throws ClientException
> >> * @since 1.2
> >> */
> >> - public native void propertyCreate(String path, String name, byte[] value,
> >> - boolean recurse, boolean force)
> >> - throws ClientException;
> >> + public void propertyCreate(String path, String name, byte[] value,
> >> + boolean recurse, boolean force)
> >> + throws ClientException
> >> + {
> >> + propertyCreate(path, name, new String(value), recurse, force);
> >> + }
> > ...
> >
> > Isn't "new String(value)" doing an implicit byte -> char conversion in
> > the JVM's default character set? Could this be lossy?
> >
> > It might be safer to make this change the other way around...
>
> Sure. The motivation for doing it this way was consistency with a
> similar construct in propertySet(). I'd be happy to change them both if
> needed.
While I'm not absolutely sure what the Right Thing is here, getting
the byte[] representation of a Java String using the JVM's default
encoding (which unfortunately might not be UTF-8), then basically
transforming each byte in that array into a character, is definitely
not correct.
Re: svn commit: r24791 - in trunk/subversion/bindings/javahl: native
src/org/tigris/subversion/javahl src/org/tigris/subversion/javahl/tests
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Daniel Rall wrote:
> On Thu, 26 Apr 2007, hwright@tigris.org wrote:
> ...
>> JavaHL: Eliminate duplicate native implementations of propertyCreate(), and
>> implement one version as a wrapper around the other. Also, modify the
>> properties test to use propertyCreate() as well as propertySet().
> ...
>> --- trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp (original)
>> +++ trunk/subversion/bindings/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp Thu Apr 26 09:00:39 2007
> ...
>> -JNIEXPORT void JNICALL
>> -Java_org_tigris_subversion_javahl_SVNClient_propertyCreate__Ljava_lang_String_2Ljava_lang_String_2_3BZZ
>> -(JNIEnv *env, jobject jthis, jstring jpath, jstring jname, jbyteArray jvalue,
>> - jboolean jrecurse, jboolean jforce)
>> -{
>> - JNIEntry(SVNClient, propertyCreate);
>> - SVNClient *cl = SVNClient::getCppObject(jthis);
>> - if (cl == NULL)
>> - {
>> - JNIUtil::throwError(_("bad C++ this"));
>> - return;
>> - }
>> - JNIStringHolder path(jpath);
>> - if (JNIUtil::isExceptionThrown())
>> - return;
>> -
>> - JNIStringHolder name(jname);
>> - if (JNIUtil::isExceptionThrown())
>> - return;
>> -
>> - JNIByteArray value(jvalue);
>> - if (JNIUtil::isExceptionThrown())
>> - return;
>> -
>> - cl->propertyCreate(path, name, (const char *)value.getBytes(),
>> - jrecurse ? true:false,
>> - jforce ? true:false);
>> -}
>> -
>> JNIEXPORT jobject JNICALL
>> Java_org_tigris_subversion_javahl_SVNClient_revProperty
>> (JNIEnv *env, jobject jthis, jstring jpath, jstring jname, jobject jrevision)
>>
>> Modified: trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java?pathrev=24791&r1=24790&r2=24791
>> ==============================================================================
>> --- trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (original)
>> +++ trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java Thu Apr 26 09:00:39 2007
>> @@ -1447,9 +1447,12 @@
>> * @throws ClientException
>> * @since 1.2
>> */
>> - public native void propertyCreate(String path, String name, byte[] value,
>> - boolean recurse, boolean force)
>> - throws ClientException;
>> + public void propertyCreate(String path, String name, byte[] value,
>> + boolean recurse, boolean force)
>> + throws ClientException
>> + {
>> + propertyCreate(path, name, new String(value), recurse, force);
>> + }
> ...
>
> Isn't "new String(value)" doing an implicit byte -> char conversion in
> the JVM's default character set? Could this be lossy?
>
> It might be safer to make this change the other way around...
Sure. The motivation for doing it this way was consistency with a
similar construct in propertySet(). I'd be happy to change them both if
needed.
-Hyrum