[Box Backup] Code in SVN -- review notes
Wed, 30 Nov 2005 20:14:06 +0000
On 29 Nov 2005, at 18:24, Martin Ebourne wrote:
> Ben Summers <email@example.com> 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
> 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.
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?