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
