On Mon, 02 Apr 2007, Hyrum K. Wright wrote:
> Daniel Rall wrote:
> > On Mon, 02 Apr 2007, hwright@tigris.org wrote:
...
> >> +/**
> >> + * 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.
Right -- I meant Doxygen in the C++ header files. Since even private
APIs are enumerated there, that means we don't have many (any?)
non-Doxygen doc strings in the C++ code.
> >> +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.
makeJFoo() works for me.
> >> + 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.
Okay. I lost the context. I'm all for replacing the exception check
with a comment for this case.
> > ...
> >> --- (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.
receiveInfo(), perhaps?
...
> Thanks for the review, Dan.
Welcome!
- Dan
- application/pgp-signature attachment: stored
Received on Wed Apr 4 03:15:09 2007