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

[Chris Wilson]

Hi Ben,

> The controversial use of goto isn't too bad in this case, but let's address 
> that later after we've got all the code in truck.

But whose truck? :-)

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

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

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

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

Lower case global functions - do you mean the emulated ones? If so, should 
I make them CamelCase and #define the originals to the replacements?

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

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

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.

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

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

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

Cheers, Chris.
-- 
_ ___ __     _
  / __/ / ,__(_)_  | Chris Wilson <0000 at qwirx.com> - Cambs UK |
/ (_/ ,\/ _/ /_ \ | Security/C/C++/Java/Perl/SQL/HTML Developer |
\ _/_/_/_//_/___/ | We are GNU-free your mind-and your software |