On Sun, 2010-05-02, Stefan Fuhrmann wrote:
> Bert Huijben wrote:
> >> Stefan Fuhrmann wrote:
> >>
> >> * subversion/include/svn_delta.h
> >> (enum svn_delta_action): ensure that these definitions
> >> will always have these values, even if new definitions
> >> were added
> >
> > This part is not necessary. These values are part of our public API
> > compatibility promise and that makes the values static for 1.X.
> >
> > The C compiler defines the same values... and if it didn't you are not
> > allowed to change the values (for the same binary compatibility reasons).
> >
> Correct. I just wanted these values to become more visible
> to anyone who might want to extend the enumeration.
>
> But I'm fine with removing that part. If the encoding should
> be changed accidentally, virtually any repository read access
> will fail with a checksum error. So, it would not linger there
> unnoticed. So, please remove that part if you apply the patch.
I compromised here by just adding a note that the svndiff implementation
relies on these values.
> >> (copy_source_ops): dito; optimize a common case
> >>
> >> * subversion/libsvn_delta/svndiff.c
> >> (decode_file_offset, decode_size): use slightly more
> >> efficient formulations
> >> (decode_instruction): directly map action codes -
> >> made possible by defining the enum values
> >
> > The enum values were already defined so that doesn't change anything (see
> > above: they can't change anyway).
> >
> O.k. omit the "- made possible ..." part.
Done.
> > Optimizing a % operator for values of 1 and 2 looks like code obfuscation to
> > me, unless you can show me some numbers that it actually makes a difference.
> > I would assume that the processor optimizes that case itself.
> >
> The performance advantage is very small anyway - no
> reason for me to put any more effort in it. Just leave it
> out from the commit.
OK, left out.
Stefan, thanks for persisting and answering all our questions.
This looks good now. I'm committing it.
Committed revision 941243.
- Julian
Received on 2010-05-05 13:03:26 CEST