[Box Backup] Code in SVN -- review notes
Tue, 29 Nov 2005 18:24:16 +0000
Ben Summers <firstname.lastname@example.org> 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
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.