zorba-coders team mailing list archive

["Paul J. Lucas" <paul@xxxxxxxxxxxxx>]

Review: Needs Fixing

In several cases where you added casts, you cast to the wrong type, e.g., the wrong size_type or something that should have been size_t.  The only reason it got rid of the warnings in those cases is because the wrong type and the correct type just so happened to be the same underlying type on Windows; but that's not necessarily the case on non-Windows platforms.

For example, in mem_streambuf.cpp, you cast size to string::size_type -- why?  Strings have nothing to do with that code.  If you looked at the man page for memcpy(3), the type of the 3rd argument is clearly size_t, so that's what you should have cast to.

I've made corrections in my own code at least.

In my code, I prefer !!expr rather than expr != 0 for conversion to bool (saves typing) -- I've changed this myself.

In places where you added #ifdef __GNUC__, it would have been nice if you fixed the indentation and included the /* __GNUC__ */ comment on the #endif -- I've fixed this myself.

As for other casts, I didn't thoroughly review them (you should do that, not me), but they looked suspicious to me:

209: It's not clear to me that the cast should be to int. It probably ought to be to whatever the type of theContent's size_type is (if it has one).

227,230,239: Here you're casting to unsigned int for theContent even though you just cast to int on line 209.  Again, this should probably be some size_type.

252: Is unsigned long really correct for bytes_in_buffer?  Is there a streamsize?

261: You fixed this line but ignored line 259 that obviously casts to a different type.

274: Is int really the right type?  It probably should be theXmlTrees's size_type (if it has one).

283,296: Should probably be the type of the iterator's type's size_type.

322: Should probably be vector<store::Item*>::size_type not int.  (To save typing, just add a typedef to the class for vector<store::Item*>.)

331,357: Should probably be theItems's type's size_type not unsigned int.

Line numbers above refer to the line numbers of the diff on the launchpad merge proposal.
-- 
https://code.launchpad.net/~zorba-coders/zorba/bug1100471/+merge/144035
Your team Zorba Coders is subscribed to branch lp:zorba.