Why not the same resulting string?

[William Payne]

Hello. Consider the two following code snippets:

/* Snippet one */
void convert_decimal_to_binary(int n,
std::string& binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
s = "1" + s;
}
else
{
s = "0" + s;
}

pos *= 2;
}
}
/* End snippet one */

/* Snippet two */
void convert_decimal_to_binary(int n,
char* binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
push_front('1', binary);
}
else
{
push_front('0', binary);
}

pos *= 2;
}
}

void push_front(char c, char* str)
{
char temp[strlen(str) + 1];

strcpy(temp, str);

str[0] = c;

strcat(str, temp);
}
/* End snippet two */

In, snippet one, when convert_decimal_to_binary() is called with n = 9,
the variable binary equals "1001" after it has completed, which is correct.
But in snippet two the variable binary equals "11010101" for the same
value for n (9). Where is the bug(s) in snippet two that causes the string
to
be incorrect?

// William Payne

 

[Victor Bazarov]

Hello. Consider the two following code snippets:

/* Snippet one */
void convert_decimal_to_binary(int n,
std::string& binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
s = "1" + s;
}
else
{
s = "0" + s;
}

pos *= 2;
}
}
/* End snippet one */

/* Snippet two */
void convert_decimal_to_binary(int n,
char* binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
push_front('1', binary);
}
else
{
push_front('0', binary);
}

pos *= 2;
}
}

void push_front(char c, char* str)
{
char temp[strlen(str) + 1];

strcpy(temp, str);

str[0] = c;

strcat(str, temp);
}
/* End snippet two */

In, snippet one, when convert_decimal_to_binary() is called with n = 9,
the variable binary equals "1001" after it has completed, which is correct.
But in snippet two the variable binary equals "11010101" for the same
value for n (9). Where is the bug(s) in snippet two that causes the string
to
be incorrect?

The "snippet two" is not supposed to compile.

char temp[strlen(str) + 1];

is not C++. To declare an array in C++ you _must_ put a _constant_
expression inside the brackets.

You should probably rewrite your 'push_front' as

void push_front(char c, char* s)
{
memmove(s + 1, s, strlen(s) + 1);
*s = c;
}

then it should work OK. Make sure that you initialise the array you
use for "binary" to contain all 0s.

Victor

 

[Alf P. Steinbach]

Hello. Consider the two following code snippets:

/* Snippet one */
void convert_decimal_to_binary(int n,
std::string& binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
s = "1" + s;
}
else
{
s = "0" + s;
}

pos *= 2;
}
}
/* End snippet one */

For this function you need a wrapper that checks for the case of
0 as well as negative value.

Also consider a wrapper for that wrapper, namely one that returns
a string instead of taking a string by reference.

Also, 'pos' would more appropriate be named 'powerOf2' or some such.

For efficiency, consider adding characters at the end of the string,
then reversing the string.

For simplicity of coding, consider using std::bitset instead of the
above.

/* Snippet two */
void convert_decimal_to_binary(int n,
char* binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
push_front('1', binary);
}
else
{
push_front('0', binary);
}

pos *= 2;
}
}

Same comments as for previous function, + this function is not very
safe: it must be called with a pointer to a sufficiently large buffer.

void push_front(char c, char* str)
{
char temp[strlen(str) + 1];

This is not standard C++, but it is allowed in C99. If you're using
a compiler that allows this by default for C++ (e.g. g++), check out
the options for enforcing standard C++. In standard C++ you could use
a std::vector<char> as temporary storage, although the best thing to
do would be to simply copy the string contents in place.

strcpy(temp, str);

str[0] = c;

strcat(str, temp);

}
/* End snippet two */

In, snippet one, when convert_decimal_to_binary() is called with n = 9,
the variable binary equals "1001" after it has completed, which is correct.
But in snippet two the variable binary equals "11010101" for the same
value for n (9). Where is the bug(s) in snippet two that causes the string
to be incorrect?

At the point where you add on 'temp' you don't have a valid one-character
string in 'str'.

 

[William Payne]

Hello. Consider the two following code snippets:

/* Snippet one */
void convert_decimal_to_binary(int n,
std::string& binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
s = "1" + s;
}
else
{
s = "0" + s;
}

pos *= 2;
}
}
/* End snippet one */

/* Snippet two */
void convert_decimal_to_binary(int n,
char* binary)
{
int pos = 1;

while(pos <= n)
{
if(n & pos)
{
push_front('1', binary);
}
else
{
push_front('0', binary);
}

pos *= 2;
}
}

void push_front(char c, char* str)
{
char temp[strlen(str) + 1];

strcpy(temp, str);

str[0] = c;

strcat(str, temp);
}
/* End snippet two */

In, snippet one, when convert_decimal_to_binary() is called with n = 9,
the variable binary equals "1001" after it has completed, which is correct.
But in snippet two the variable binary equals "11010101" for the same
value for n (9). Where is the bug(s) in snippet two that causes the string
to
be incorrect?

The "snippet two" is not supposed to compile.

char temp[strlen(str) + 1];

is not C++. To declare an array in C++ you _must_ put a _constant_
expression inside the brackets.

You should probably rewrite your 'push_front' as

void push_front(char c, char* s)
{
memmove(s + 1, s, strlen(s) + 1);
*s = c;
}

then it should work OK. Make sure that you initialise the array you
use for "binary" to contain all 0s.

Victor

Thanks alot for your help, Victor, your code works great. I also thank you
for pointing out my violation of the C++ standard, my compiler (GCC 3.1.1)
didn't complain about it, but now I know that I should avoid writing such
statements.

// William Payne

 

[Kevin Goodsell]

Thanks alot for your help, Victor, your code works great. I also thank you
for pointing out my violation of the C++ standard, my compiler (GCC 3.1.1)
didn't complain about it, but now I know that I should avoid writing such
statements.

It will complain if you use the right options. I don't know what they
are off the top of my head, though. -Wall and -pedantic should probably
be in there, but there's also one that tells it which standard to use,
like -ansi, but I think that's for C.

-Kevin

 

[William Payne]

It will complain if you use the right options. I don't know what they
are off the top of my head, though. -Wall and -pedantic should probably
be in there, but there's also one that tells it which standard to use,
like -ansi, but I think that's for C.

-Kevin

I meant version 3.3.1 and the code was compiled with -W and -Wall.

// William Payne

 

[Kevin Goodsell]

I meant version 3.3.1 and the code was compiled with -W and -Wall.

Even so, it was (I believe) still compiling in "GNU C++" mode. You can
make it "Standard C++" mode with some switch, but I don't know what it
is. If you look up -ansi, it will probably give you a list of similar
switches, and one (or more) should be for standard C++ mode. It's
probably something like --std=c++98 (IIRC, -ansi is equivalent to
--std=c89).

-Kevin

 

[Kevin Saff]

void push_front(char c, char* str)

A char*, huh. I'll call this with '1' and "001" and see what happens...

{
char temp[strlen(str) + 1];

strcpy(temp, str);

OK, now temp == "001".

str[0] = c;

**
Now str == "101", since you just wrote over the first character.

strcat(str, temp);

Appends temp to str, giving "101001". Not what you expected? Well it could
be even worse. If I call this with '0' and str == "", then str has no
null-terminator after str[0] = '0', so strcat can do anything it wants.

I guess you could add the line str[1] = '\0'; at the ** above, and it
might work. I would just use a string myself.