[Box Backup-dev] Moving trunk to release

[Martin Ebourne]

On Fri, 2006-01-06 at 21:40 +0000, Chris Wilson wrote:
> Fundamental type changes and missing includes, which might break some 
> platforms, but look correct to me, can be seen with:
> 
>    svn diff
>    http://bbdev.fluffy.co.uk/svn/box/chris/win32/vc2005-compile-fixes@254
>    http://bbdev.fluffy.co.uk/svn/box/chris/win32/type-changes
> 

Finally got chance to review these, sorry it took so long. Also
apologies if Ben already covered these & I forgot.

I've got comments on several of the individual changes as per below:

        Index: lib/backupclient/BackupClientRestore.cpp
        ===================================================================
        --- lib/backupclient/BackupClientRestore.cpp	(.../vc2005-compile-fixes)	(revision 254)
        +++ lib/backupclient/BackupClientRestore.cpp	(.../type-changes)	(revision 326)
        @@ -101,7 +101,7 @@
         			// ID
         			rWrite.Write(&mNextLevelID, sizeof(mNextLevelID));
         			// Name string
        -			int32_t nsize = mNextLevelLocalName.size();
        +			std::string::size_type nsize = mNextLevelLocalName.size();
         			rWrite.Write(&nsize, sizeof(nsize));
         			rWrite.Write(mNextLevelLocalName.c_str(), nsize);
         			// And then the level itself
        

Incorrect because the corresponding load is unchanged:
			int32_t nsize = 0;
			CHECKED_READ(&nsize, sizeof(nsize));


        Index: lib/server/Protocol.h
        ===================================================================
        --- lib/server/Protocol.h	(.../vc2005-compile-fixes)	(revision 254)
        +++ lib/server/Protocol.h	(.../type-changes)	(revision 326)
        @@ -94,7 +94,7 @@
         	void Read(int32_t &rOut);
         	void Read(int16_t &rOut);
         	void Read(int8_t &rOut);
        -	void Read(bool &rOut) {int8_t read; Read(read); rOut = (read == true);}
        +	void Read(bool &rOut) {bool read; Read(read); rOut = (read == true);}
         	void Read(std::string &rOut);
         	template<typename type>
         	void Read(type &rOut)
        

This is clearly wrong - infinite loop!



        Index: lib/common/CollectInBufferStream.h
        ===================================================================
        --- lib/common/CollectInBufferStream.h	(.../vc2005-compile-fixes)	(revision 254)
        +++ lib/common/CollectInBufferStream.h	(.../type-changes)	(revision 326)
        @@ -52,7 +52,7 @@
         	MemoryBlockGuard<char*> mBuffer;
         	int mBufferSize;
         	int mBytesInBuffer;
        -	int mReadPosition;
        +	pos_type mReadPosition;
         	bool mInWritePhase;
         };
        

What about mBytesInBuffer?



        Index: lib/common/FdGetLine.cpp
        ===================================================================
        --- lib/common/FdGetLine.cpp	(.../vc2005-compile-fixes)	(revision 254)
        +++ lib/common/FdGetLine.cpp	(.../type-changes)	(revision 326)
        @@ -151,8 +151,8 @@
         	else
         	{
         		// Check for comment char, but char before must be whitespace
        -		int end = 0;
        -		int size = r.size();
        +		int end = 0; // can be negative
        +		std::string::size_type size = r.size();
         		while(end < size)
         		{
         			if(r[end] == '#' && (end == 0 || (iw(r[end-1]))))


Comparison between possible signed and unsigned - not good. Maybe change
algorithm so end cannot go -ve?



        Index: lib/common/MemBlockStream.h
        ===================================================================
        --- lib/common/MemBlockStream.h	(.../vc2005-compile-fixes)	(revision 254)
        +++ lib/common/MemBlockStream.h	(.../type-changes)	(revision 326)
        @@ -45,8 +45,8 @@
         private:
         	const char *mpBuffer;
         	int mBytesInBuffer;
        -	int mReadPosition;
        +	pos_type mReadPosition;
         };
         
         #endif // MEMBLOCKSTREAM__H
        

Again, mBytesInBuffer too.


Re: The various changes to long long

Box currently doesn't use long long anywhere, and it's a language
extension so I'm uneasy about relying on it. How about using (u)int64_t?


All the other changes look correct.

I'll try to get to your other windows changes soon.

Cheers,

Martin.