Daniel Rall wrote:
> On Mon, 02 Apr 2007, hwright@tigris.org wrote:
> ...
>> --- (empty file)
>> +++ trunk/subversion/bindings/java/javahl/native/InfoCallback.cpp Mon Apr 2 14:53:06 2007
> ...
>> +/**
>> + * Destroy a InfoCallback object
>> + */
>> +InfoCallback::~InfoCallback()
>> +{
>> + // the m_callback does not need to be destroyed, because it is the passed
>> + // in parameter to the java SVNClient.blame method.
>> +}
>
> Cut-n-paste of SVNClient.blame.
Fixed in r24358.
> ...
>> +/**
>> + * Callback called for a single path
>> + * @param path the path name
>> + * @param pool memory pool for the use of this function
>> + */
>
> We don't want tool-formatted doc strings in the .cpp file, only in the
> header file. And there, we should probably be using Doxygen markup,
> rather than the JavaDoc that's crept in.
I removed the JavaDoc strings in r24358. These are probably considered
private functions, from an API perspective, and we've generally haven't
used Doxygen for non-public functions. Given that, I didn't bother to
Doxygen-ify them. If we decide to do that, we should probably do it to
all functions in the C++ layer that need it at once, instead of piecemeal.
>> +svn_error_t* InfoCallback::singleInfo(const char *path,
>> + const svn_info_t *info,
>> + apr_pool_t *pool)
>> +{
> ...
>> + info_entry infoEntry;
>> + SVN_JNI_ERR(createInfoEntry(infoEntry, path, info, pool), SVN_NO_ERROR);
>> + jobject jinfo2 = createJavaInfo2(&infoEntry);
>
> We have two naming conventions going on here:
>
> a) makeJFoo
> b) createJavaFoo
>
> I don't mind which we use, but would prefer to make things consistent.
+1 for consistency. Let's decide which one we want to use, and then do
a mass change across the C++ layer. I like makeJFoo, if for no other
reason than that it is shorter.
>> + env->CallVoidMethod(m_callback, mid, jinfo2);
>> + // Return SVN_NO_ERROR here regardless of an exception or not.
>
> What do we do with any Java exception that's thrown?
The alternative here is:
env->CallVoidMethod(m_callback, mid, jinfo2);
if (JNIUtil::isJavaExceptionThrown())
return SVN_NO_ERROR;
return SVN_NO_ERROR;
The if statement is clearly redundant; in fact we've eliminated similar
code in other places recently. The comment is meant to clear confusion
regarding that fact, though it could arguably be better. As of r24358,
we're now checking for the exception explicitly in this specific case,
because there is now a DeleteLocalRef() after it, but the redundancy
still holds for the last check of the function.
> ...
>> --- (empty file)
>> +++ trunk/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/InfoCallback.java Mon Apr 2 14:53:06 2007
> ...
>> +/**
>> + * this interface is used to retrieve info each result in a
>> + * SVNClientInterface.info2 call.
>> + */
>> +public interface InfoCallback
>> +{
>> + /**
>> + * the method will be called for every line in a file.
>> + * @param info the Info2 object
>> + */
>> + public void singleInfo(Info2 info);
>> +}
>
> I'm not a big fan of this method name (nor singleLine, FWIW). Better
> suggestions?
Good point. I don't have any suggestions handy, but if we do change the
name, we should carry that down to the C++ layer as well.
> ...
>> --- trunk/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java (original)
>> +++ trunk/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java Mon Apr 2 14:53:06 2007
> ...
>> + public Info2[] info2(String pathOrUrl, Revision revision,
>> + Revision pegRevision, boolean recurse)
>> + throws ClientException
>> + {
>> + MyInfoCallback callback = new MyInfoCallback();
>> +
>> + info2(pathOrUrl, revision, pegRevision, recurse, callback);
>> + return callback.getInfoArray();
>> + }
> ...
>> +
>> + /**
>> + */
>
> Missing JavaDoc. Should indicate that instances of this class aren't
> thread-safe.
Done in r24357.
>> + private class MyInfoCallback implements InfoCallback
>> + {
>> + private List infos = new Vector();
>
> As this Collection doesn't need to be thread-safe, you should use an
> ArrayList instead of a Vector.
Done in r24357.
>> + public void singleInfo(Info2 info)
>> + {
>> + infos.add(info);
>> + }
>> +
>> + public Info2[] getInfoArray()
>> + {
>> + Info2[] infoArray = new Info2[infos.size()];
>> + Iterator it = infos.iterator();
>> + int i = 0;
>> +
>> + while (it.hasNext())
>> + {
>> + infoArray[i] = (Info2) it.next();
>> + i++;
>> + }
>> +
>> + return infoArray;
>
> You can replace this entire method body with a single line:
>
> return (Info2 []) infos.toArray(new Info2[infos.size()]);
>
> http://java.sun.com/j2se/1.3/docs/api/java/util/Collection.html#toArray(java.lang.Object[])
Ah. I'm still trying to get back to fluency with the Java API. :)
I've changed this class, as well as a very similar construct above it in
r24357.
>> + }
>> + }
>> }
> ...
Thanks for the review, Dan.
-Hyrum
Received on Tue Apr 3 05:32:51 2007