Re: JavaHL bindings cleanup

On Fri, 16 Mar 2007, Blair Zajac wrote:

> Hyrum K. Wright wrote:
> >Blair Zajac wrote:
> >>Hyrum K. Wright wrote:
> >>>Mark Phippard wrote:
> >>>>On 3/16/07, Hyrum K. Wright <hyrum_wright@mail.utexas.edu> wrote:
> >>>>>I've been mucking around in the JavaHL bindings the past few days,
> >>>>>and I
> >>>>>have the following suggestions:
> >>>>>
> >>>>>1) Update the .cpp and .java files to use the same coding style that
> >>>>>the
> >>>>> main codebase uses.
> >>>>I am not sure it makes sense to force the .java files to follow a style
> >>>>used
> >>>>in C/C++, but to be honest I am not familiar with the scope of what that
> >>>>would mean. Maybe not much. For me, since I use tools like Eclipse, I
> >>>>would just want the style to be something reasonable that I can set in
> >>>>Eclipse and let it handle for me.
> >>>That's understandable. The .java code is different enough from the .cpp
> >>>code that it shouldn't matter too much. The problem I'm trying to solve
> >>>is two-fold: the .cpp files have a style all their own, and it isn't
> >>>consistent; and, C++ is similar enough to C that when working in them,
> >>>it would be easier if they just had the same style.

+1

> >>>Would moving function parameters to individual lines be incongruent with
> >>>prevailing Java styles?

Usually, yes. I'd list as many params as possible on the same line up
to 78 chars or so.

> >>>>2) Rearrange methods within classes in alphabetical order, with older
> >>>>> methods first in the case of overloaded methods.

-0. If anything, I'd rather see newer methods first. I don't see
this as a compelling change one way or the other, though.

> >>>>>3) Implement older APIs as wrappers around newer APIs in the same
> >>>>>class.
> >>>>> (Currently, some are implemented as wrappers around newer APIs in
> >>>>> other classes.)
> >>>>>
> >>>>>I think this would improve maintainability substantially, but I wanted
> >>>>>to know what others thought. Comments? Objections?
> >>>>In general I am fine with the concept. I do not know what #3 means at a
> >>>>technical level so cannot comment. Obviously the main issue is that the
> >>>>changes do not make it too hard to backport future fixes, but I would
> >>>>say
> >>>>that the number of those are likely to be a number approaching zero. So
> >>>>better to do this now, well in advance of a 1.5 branch.
> >>>Other than giving the compiler/optimizer a bit more information, #3,
> >>>doesn't have much technical impact. This is mainly limited to the
> >>>SVNClientSynchronized class, which can implement old APIs using newer
> >>>ones in the same class, or by calling the new API in SVNClient. It just
> >>>seems Right to use the API in the same class whenever it is available.
> >>Can you give an example with the actual method names? I'd like to see
> >>what you're referring to.
> >
> >Look at the two remove() methods in SVNClientSynchronized. The old
> >version calls the new version directly.
> >
> >Now, look at the two add() methods in SVNClientSynchronized. They both
> >call the corresponding versions in SVNClient.
> >
> >Functionally, the two behaviors are the same: they both end up calling
> >the correct native code. It just seems like there should just be one
> >call to SVNClient, and that SVNClientSynchronized APIs should wrap each
> >other, where possible.
>
> Yes, I agree, they should be consistent. Definitely easier to read the
> code when SVNClientSynchronized.remove() calls the other
> SVNClientSynchronized.remove() then both calling SVNClient.remove().

I'd like to see a consistent pattern used. I don't have much of a
preference as to which pattern is followed.