[Box Backup-dev] COMMIT r348 - in box/chris/bb-save-state: . bin/bbackupd lib/common lib/server

[Martin Ebourne]

Ben Summers <ben@fluffy.co.uk> wrote:
> I'm probably being too picky here. I wonder what Martin thinks.

You asked for my opinion so - uh oh! - you get it!

No, I don't think you're being too picky, it's just unfortunate this 
has landed on Chris as the only person managing to spend time on it at 
the moment.

>> svn diff -r 336:349 http://bbdev.fluffy.co.uk/svn/box/chris/bb-save- state

I reviewed 336:350 to catch the new changes.

> I would have removed the operator overloading from Archive, and  
> personally would have just required that classes derived from Archive 
>  only provide Read(void*,int length) and Write(void*,int length)  
> functions, rather than doing absolutely everything, and made base  
> class Get and Put inline functions which call those.

I like your second plan rather than this one. Seems Chris got the wrong 
end of the stick on this one. Fortunately it's all good because what he 
did is closer to your second plan anyway.

> 	-- #include "Archive.h" -> class Archive;  ?

Agreed.

> As I understand it, I just need to adjust the infrastructure building 
>  to run ./bootstrap, and then include all the new files from SVN in  
> the root, along with the generated configure?

Not sure what you mean by root. There's new files in subdirs too. I 
think you'll do the right thing though.

> Onto the code... I wonder if it would be better to just delete the  
> Archive class, and rename IOStreamArchive to Archive. I don't quite  
> see what this gives us, except added complexity and the overhead of  
> virtual function calls, given that you can get all the flexibility  
> you need by simply choosing your IOStream appropriately. Then Archive 
>  becomes a very simple class, calls to which compile down to calls to 
>  the underlying IOStream object.

I agree completely. The abstraction for Archive is at the wrong level 
anyway. Thinking about possible alternatives you might want to slot in 
here and an obvious one that springs to mind is a database store(*). 
Using the current Archive interface you couldn't do that. Really the 
Archive interface is an IO interface and as such should just be 
replaced with an IO stream.

It's further confused by the Box IOStream implementation. Although it's 
called an IOStream, when compared to standard C++ streams it's really a 
streambuf. Protocol is more like the C++ iostream.

Long term I think there'd be mileage in refactoring Archive and 
Protocol to remove duplication, but that's way too much work at the 
moment.

Short term what Ben suggests should be fairly simple and keep overhead 
down. It's also moving in the right direction for the long term plans.

> Also, in my experience, defining virtual functions inline in header  
> files causes problems with compilers. I'm sure MSVC has been fixed  
> now, but still... :-)

A good rule of thumb is to never define virtual methods as inline. If 
your methods are virtual it should be because you're using a 
polymorphic interface, in which case the compiler can't inline them 
anyway. Sometimes there are reasons to use virtual inline, but if you 
stick to the rule of thumb you'll be right 99% of the time.

Even ignoring broken compilers, it creates headaches for the compiler 
if you declare virtual methods inline (and can result in code 
duplication etc).

> OTOH, perhaps we should just merge it and get on with things?

I think it would be great if Chris can make the fixes you suggest since 
at this stage they shouldn't be too tricky. On the other hand if he's 
had enough already then they aren't his changes and we should be 
thankful for the work he's already put in. :)

In terms of changes still not in trunk, I don't think the type changes 
have been merged yet. I think Chris did fix some/all of them following 
review comments though. Have we a new diff to review for that one 
before merge?

And are some windows changes still pending?

(*) Sometime in the future it might make sense to replace this custom 
state file and the whole of Berkeley DB hell with sqlite, which I think 
would be much more manageable and give us added flexibility for new 
features (maybe inotify).

Cheers,

Martin.