while(*(s1++)=*(s2++))

[kissilev]

Hii , I have this function that concatenate two strings :
void string_cat2(char *s1,char *s2){
while(*s1) ++s1;
while(*(s1++)=*(s2++));
}

my question is about the second "while" , what is the order of operation inside the while.....I don't understand when *s1 gets its NULL...

[Computergeek01]

Maybe I've reached my mental limit for the day but... WTF are you trying to do? All I see is you screwing around with pointer reassignment. The first while() loop will never be false, because it's as good as saying while(1), so what you are doing it seems is trolling the stack. Are s1 or s2 even pointing to an array? If possible please post more relavent code.

To answer your question to the best of my over caffinated ability You are scrolling through the array with the second while() loop, and while there is another element in the array it seems you are trying to also scroll through s2. I think. So the OOO is first increase value stored in s1 (which is a memory address) by one, thereby pointing it to the very next memory address which would be the next char in the array. You need to test s1 for NULL in this code in order to exit the while() loop. A better way might be passing the size of the array that s1 and s2 are pointing to and use that as your counter.

[tummychow]

I'm all for brevity but the way that code is written makes it illegible and incomprehensible.

[kissilev]

Computergeek01,I realy don't understand you...
This is the entire code.....the function is working fine!!!

#include <stdio.h>
#include <string.h>
#define MAX_LENGTH 81

/*Option #1*/
void string_cat(char *s1, char *s2)
{
while (*s1 ) ++s1;/*We could equivelently have written: s1++; */
do
{
*s1 = *s2;
s1++;
s2++;
}while(*s2);

*s1 = '\0';
}
/*Option #2*/
void string_cat(char *s1, char *s2)
{
while (*s1 ) ++s1;
while(*(s1++) = *(s2++));
}

/*Option #3*/
void string_cat(char *s1, char *s2)
{
strcpy(s1+strlen(s1),s2);
}


void main()
{
char str1[MAX_LENGTH], str2[MAX_LENGTH];
printf("Enter a string with maximum %d characters:",(MAX_LENGTH-1)/2);
gets(str1);
printf("Enter a string for concatination with maximum %d characters:",(MAX_LENGTH-1)/2);
gets(str2);
string_cat(str1,str2); /*We can use any (but not more than one) of the options above! */
printf("The final string is \"%s\" \n",str1);
}




Option 2 works just like option1 but I am tring to understand option 2!!!!!
So please dont tell me that the first while loop will never be false because its wrong....

[tummychow]

Needs code tags.

[Computergeek01]

Ok then, if it works I won't argue I'm too lazy to compile and run it myself.

So to answer your question you are first setting the pointer to the delimiting character in the array pointed to by s1 with the first while() loop in Option 2. Then the second while() loop is selecting the next memory address set aside in the stack for s1 and adding the contents of the memory address pointed to by s2 to s1 while shifting the s2 and s1 pointer to the next memory address for every iteration of the while() loop. In effect you are adding the contents of each element in the second array to memory addresses belonging to the first array.

What is that last bit about *s1 getting its NULL in your first post?

Does this work? I don't see you over riding the delimiting character in the first array so I would imagine that the compiler doesn't output the rest of the array ie the contents of s2.

[kissilev]

Yes its work.....I have compiled it , all options works Identically.. the second while loop over riding the '\0' of s1 , in option#1 you have to assign *s1='\0' otherwise you will get a lot of garbage , this is exactly my problem with the second option because there I don't see how *s1 gets its NULL , string you have to delimitaion with NULL .

[helios]

If you exploded the second while, it would look like this:

//while (*(s1++)=*(s2++));
label:
{
    char *temp1=s1,
         *temp2=s2;
    s1+=1;
    s2+=1;
    *temp1=*temp2;
    if (!(*temp1==0))
        goto label;
}

[Computergeek01]

Ew goto...j/k helios is a good guy. Anyway I belive that s1 gets its NULL delimiter from s2. Your code looks like it copies everything in the s2 array to the end of the s1 array so this would include the delimeter, it appears that since you don't stop this process though and your arrays are the same size you are acctually writting past the end of the array s1 is pointing to, this isn't good practice. Someone correct me if I'm wrong...

Is something not working right? Or is this a general question to understand the guts of it all?

[chrisname]

It's very easy to write illegible code, but it isn't difficult to make legible code:

void string_cat2(char *s1,char *s2){
while(*s1) ++s1;
while(*(s1++)=*(s2++));
}

becomes

void string_cat2(char *s1, char *s2) {
    for (; *s1 != 0; ++s1)
        /* Do nothing */;

    for (; *s1 != 0 && *s2 != 0; s1++, s2++)
        *s1 = *s2;
}

which is a lot easier to read. Just because it takes up more lines, doesn't mean it's worse. At least you can read it, which makes it easier to debug.

Edit: by the way, prefix increment won't help you there. IIRC it's only really faster for objects, and only in C++. Elsewhere it's pointless except when doing some kind of voodoo with pointers and arrays.

@Computergeek01,
Taken from linux-2.6.32.8/Documentation/CodingStyle:

Chapter 7: Centralized exiting of functions

Albeit deprecated by some people, the equivalent of the goto statement is
used frequently by compilers in form of the unconditional jump instruction.

The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.

The rationale is:

- unconditional statements are easier to understand and follow
- nesting is reduced
- errors by not updating individual exit points when making
modifications are prevented
- saves the compiler work to optimize redundant code away ;)

int fun(int a)
{
        int result = 0;
        char *buffer = kmalloc(SIZE);

        if (buffer == NULL)
                return -ENOMEM;

        if (condition1) {
                while (loop1) {
                        ...
                }
                result = 1;
                goto out;
        }
        ...
out:
        kfree(buffer);
        return result;
}

[helios]

It's not really illegible. It's fairly easy to understand if you're familiarized with the language. What are you doing reading other people's code if you aren't, anyway?

[moorecm]

Looks more like personal preference to me. I would prefer the more concise one...

[kbw]

The code is an almost standard strcat implementation.

Although you may find it confusing, for those familiar with that style it's quite clear. In fact, Copien's Advanced C++ Programming Styles and Idioms gives a strcpy version as an example of obvious code.

[chrisname]

@helios,
It's easier to read when it's formatted with indentation and what-not. At any rate, I think I'm pretty familiar with the language... I may not have a lot of experience, but I probably understand the syntax of C/C++ as much as I'm ever going to.

@moorecm,
Maybe you would. All I did was indent with code tags, and change some operators around. Personally, I think my version is easier to read. Things like this
while ((*(char*)dest++ = *(char*)src++) != 0);
don't sit well with me.

@kbw,
It wasn't the style that I found hard to read. It was the fact there was no indentation and there were operators all over the place.

[helios]

Personally, I find this
while (*(s1++)=*(s2++));
is easier to read than this

for (; *s1 != 0 && *s2 != 0; s1++, s2++)
    *s1 = *s2;

The fewer tokens, the faster you'll figure out what something is doing.

[jsmith]

It's also less buggy than yours. :)

[chrisname]

I guess that's what I get for rushing. Lesson learned.