[Box Backup] Code in SVN -- review notes

[Ben Summers]

On 29 Nov 2005, at 18:24, Martin Ebourne wrote:

> 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!

Sorry.

>
> 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.

That would be very useful, thanks.

>
> 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.)

Those two should be fixed.

>
> All looks ok otherwise though.

Yes, I think it should be fine. We can then start testing it properly.

It is the Win32 stuff which bothers me, and needs a quite a bit of  
time to test out properly. I think we've merged it into a 0.09  
release now, though?

Ben