[Box Backup-dev] Reviewing code -- help!

[Ben Summers]

On 7 Dec 2005, at 16:13, Chris Wilson wrote:
>
>> WinNamedPipe should be an IOStream derived class, then use  
>> IOStreamGetLine not PipeGetLine.
>>
>> WinNamedPipe::Write may not work in edge cases where it doesn't  
>> write all the bytes given.
>
> OK, will do as soon as I can.

Thanks.

>
>> The .bat, config and installer files should not be in lib/win32 as  
>> they're not generic win32 stuff. I suggest bin/bbackupd/win32
>
> Sure, that should be easy. Although people might not find it easily  
> down there. Perhaps docs/win32?

They're not really documents either. Surely the build system will be  
packaging them up? Perhaps they could be included in the parcel  
making script?


>
>> If any code is copied and pasted from sources, can there be a note  
>> of where it came from? I know Nick did it from some BSD licensed  
>> code, though.
>
> I always do this - the only things I can think of are the  
> customised m4 templates (where I left the original copyright notice  
> in) and TemporaryDirectory, where I provided a link to the MSDN  
> article that it came from.

I think some of the structure definitions in emu.h must have come  
from somewhere.


>
>> Not keen on the global and member function names beginning with  
>> lower case letters (and other style "violations").
>
> Lower case members should go away when WinNamedPipe and PipeGetLine  
> are merged with the Unix versions, SocketStream and IOStreamGetLine.

Yes.

>
> Lower case global functions - do you mean the emulated ones?

No, obviously those are fine.

> If so, should I make them CamelCase and #define the originals to  
> the replacements?

I think this was in the threading code. Maybe I was imagining it,  
there's a lot there.

>
>> But really, I think it better to get it in the tree and fix it up  
>> later, rather than delay any longer. But it would be nice if the  
>> first three could be address beforehand.
>
> I'll do my best. I'm really pleased that people have been generally  
> positive about getting the port merged sooner rather than later.

As long as it only breaks Win32, not POSIX... :-)

>
>> this change:
>>
>> -AX_CHECK_MOUNT_POINT(, [AC_MSG_ERROR([[cannot work out how to  
>> discover mount points on your platform]])])
>> +AX_CHECK_MOUNT_POINT([],[])
>>
>> I'm not an autoconf sort of person, but that looks worrying to me?  
>> But otherwise there's only minor things.
>
> If you mean the fact that I disabled the fatal error - Win32 has no  
> mount points, as far as I know, so if autoconf bombs out then you  
> can't build.

But doesn't that break other platforms?

>
> I couldn't see an easy way at the time to disable this check just  
> for Win32, although I'm learning and I have some ideas now.
>
> If you mean the empty strings [], then I don't know. I thought it  
> looked better than entirely missing arguments. However, I don't  
> know much about Autoconf's quoting rules, perhaps someone can  
> correct me.

I hope Martin will come up with the answer.


>
>> svn diff http://bbdev.fluffy.co.uk/svn/box/chris/win32/merge/02-  
>> compile-on-cygwin/ http://bbdev.fluffy.co.uk/svn/box/chris/win32/  
>> merge/03-compile-with-mingw-on-win32/
>>
>> I'm not convinced about this one, mainly because it has a load of  
>> GPL code in there. The other changes seem minor though.
>
> The GPL code being [config.guess], [config.sub] and [infrastructure/ 
> m4/ax_check_libs_prototype.m4]?
>
> [config.guess] and [config.sub] are from Automake 1.8. They are  
> needed to guess the platform type. If we don't include them  
> ourselves then we'll have to find a way to copy them from the  
> user's automake installation, or rewrite them from scratch.

If they're part of the base install, surely you don't need to include  
a copy in the project?

>
> [infrastructure/m4/ax_check_libs_prototype.m4] is needed to detect  
> functions on Win32. I could just make the test that calls nanosleep  
> call Sleep instead on WIN32, and remove this.

Sounds sensible.

>
> Since these are quite small blocks of code, perhaps we can apply to  
> the authors for a license exemption?

I think Martin will come up with a good plan. (I hope.)

Ben