Evil for loop
-
I did some refactoring of some older code and came across this snippet:
std::string str("This is a test str"); for(int i=str.length()-1;i>=0;i--) { printf("Examining char %d",i); }The compiler complained about this with a warning: warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data Being a fan of zero compiler warnings I did give him what he wanted and changed the type of i from int to size_t. Ok it compiles now without any warnings but this change broke many applications during runtime. I was the bad guy of the week because this change was made in a low level library.NEVER refactor without unit tests...
-
I did some refactoring of some older code and came across this snippet:
std::string str("This is a test str"); for(int i=str.length()-1;i>=0;i--) { printf("Examining char %d",i); }The compiler complained about this with a warning: warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data Being a fan of zero compiler warnings I did give him what he wanted and changed the type of i from int to size_t. Ok it compiles now without any warnings but this change broke many applications during runtime. I was the bad guy of the week because this change was made in a low level library.C4267 is one of those 64bit compatability warnings.
size_tis anunsigned inton Win32 but it is anunsigned __int64on Win64. It used to be thatsizeof(int)was OS dependant (2 bytes on Win16, 4 bytes on Win32) but from now onsizeof(int)is fixed at 4 bytes andsizeof(size_t)is variable depending on the OS. Now many CRT functions that used to work withints now work withsize_ts. This stupid change by MS is sure to break a lot of code when it is ported to Win64.
You may be right I may be crazy -- Billy Joel -- Within you lies the power for good - Use it!
-
I did some refactoring of some older code and came across this snippet:
std::string str("This is a test str"); for(int i=str.length()-1;i>=0;i--) { printf("Examining char %d",i); }The compiler complained about this with a warning: warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data Being a fan of zero compiler warnings I did give him what he wanted and changed the type of i from int to size_t. Ok it compiles now without any warnings but this change broke many applications during runtime. I was the bad guy of the week because this change was made in a low level library.This is one of the reasons I prefer to use a reverse iterator in these "reverse" loops.
-
NEVER refactor without unit tests...
I agree. But I must also add: NEVER refactor when not ALL Unit Tests are green. Failing to do so will get you into the bad habit that some test are ok to fail and you miss the new important failed test case.
-
I did some refactoring of some older code and came across this snippet:
std::string str("This is a test str"); for(int i=str.length()-1;i>=0;i--) { printf("Examining char %d",i); }The compiler complained about this with a warning: warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data Being a fan of zero compiler warnings I did give him what he wanted and changed the type of i from int to size_t. Ok it compiles now without any warnings but this change broke many applications during runtime. I was the bad guy of the week because this change was made in a low level library.I would've expected a compiler warning there about testing an unsigned variable to be >= 0.
--Mike-- Visual C++ MVP :cool: LINKS~! Ericahist | PimpFish | CP SearchBar v3.0 | C++ Forum FAQ
-
I did some refactoring of some older code and came across this snippet:
std::string str("This is a test str"); for(int i=str.length()-1;i>=0;i--) { printf("Examining char %d",i); }The compiler complained about this with a warning: warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data Being a fan of zero compiler warnings I did give him what he wanted and changed the type of i from int to size_t. Ok it compiles now without any warnings but this change broke many applications during runtime. I was the bad guy of the week because this change was made in a low level library.More importantly, what was the loop actually doing? (I'm assuming you simplified the code for display purposes here). The reason being ... a custom loop may not have been needed at all.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
More importantly, what was the loop actually doing? (I'm assuming you simplified the code for display purposes here). The reason being ... a custom loop may not have been needed at all.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
You are right that the code posted here was simplified. I have posted it to demonstrate this specific problem. The loop was needed but this variable hoisting optimization did ruin my day. Yours, Alois Kraus
-
I would've expected a compiler warning there about testing an unsigned variable to be >= 0.
--Mike-- Visual C++ MVP :cool: LINKS~! Ericahist | PimpFish | CP SearchBar v3.0 | C++ Forum FAQ
That's the reason the change from int to size_t broke the code, right? By testing for when an unsigned variable becomes negative, it becomes stuck in an infinite loop. I don't see any other plausible reason for such a simple change to break code. Besides that, you shouldn't have to do reverse iteration. Wouldn't the compiler figure out that it's more efficient to iterate in reverse, and automagically add the appropriate code? I'm recalling a quote by some Knuth guy :) about premature optimization and the root of all evil... I mean, if they're going to the trouble to reverse iterate, why not cache string.length() into an int? One might think that repeatedly calling string.length() is going to be detrimental to performance...
-
I did some refactoring of some older code and came across this snippet:
std::string str("This is a test str"); for(int i=str.length()-1;i>=0;i--) { printf("Examining char %d",i); }The compiler complained about this with a warning: warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data Being a fan of zero compiler warnings I did give him what he wanted and changed the type of i from int to size_t. Ok it compiles now without any warnings but this change broke many applications during runtime. I was the bad guy of the week because this change was made in a low level library. -
str.length() returns an unsigned int.(in MSCRT) so, it's unreliable when you do subtration on it. you should do like below: for (size_t i = 0; i < str.length(); i ++) { printf("Examining char %d",str.length() -i); }
//////////////////////////////
G
willbin wrote:
for (size_t i = 0; i < str.length(); i ++) { printf("Examining char %d",str.length() -i); }
That's another version that should cache the length... it uses the (unchanging) value _twice_ per iteration.
-
That's the reason the change from int to size_t broke the code, right? By testing for when an unsigned variable becomes negative, it becomes stuck in an infinite loop. I don't see any other plausible reason for such a simple change to break code. Besides that, you shouldn't have to do reverse iteration. Wouldn't the compiler figure out that it's more efficient to iterate in reverse, and automagically add the appropriate code? I'm recalling a quote by some Knuth guy :) about premature optimization and the root of all evil... I mean, if they're going to the trouble to reverse iterate, why not cache string.length() into an int? One might think that repeatedly calling string.length() is going to be detrimental to performance...
ITGFanatic wrote:
By testing for when an unsigned variable becomes negative, it becomes stuck in an infinite loop. I don't see any other plausible reason for such a simple change to break code.
Accessing memory beyond that which was allocated for the string.