[Box Backup] Code in SVN -- review notes

[Martin Ebourne]

Ben Summers <ben@fluffy.co.uk> wrote:
> I've taken a look at the code checked in recently, notes below. Does  
> anyone else feel like reviewing the code?
> Generally, it's almost ready to merge in, and some handy stuff to  have.

After the initial flurry of activity everything seems to have gone 
quiet again!

It'd be nice to get at least a beta out before xmas. Not sure what the 
next steps are. Can we go and merge chromi's & my changes onto trunk 
and make it work? I don't think it matters much which order they are 
merged in, and I'm happy to do the merging if others like.

I've had a look at chromi/diffopt and here's my review comments here:

- Return values of functions changed to be const where the return type 
will be an rvalue anyway. Hence const doesn't add anything because it 
is already const. Or was the plan to make the method const? Looks like 
most of them could be, in which case const is in the wrong place.

- Use of goto is incorrect. ISO C++ says goto is only valid within a 
block, not between blocks. It works in g++, but is incorrect C++ and 
should be avoided. Also, although goto is occasionally handy in C, I've 
yet to see a use of goto in C++ that couldn't be made more maintainable 
by removing it and refactoring the code. (Splitting into helper 
functions may be an improvement here.)

All looks ok otherwise though.

Cheers,

Martin.