string parsing issue

[Chervil]

As you already mentioned, strncpy() will copy the part of the string that you need for you. You need to take three steps.

1. make sure that you have a suitably sized character array which will be the destination. (allow for the null terminator).

2, do the copy, using the length calculated as p2-p1 as the third parameter.

3. under some circumstances the strncpy may not supply a null terminator, so it is probably a good idea to add it yourself after the call to strncpy.

[aliyesami]

i modified the code and i am not seeing where iam doing wrong , its working but eating the last integer , and printing 7 instead of 8 integers .

char * p = recvd_chars;
char * p3 = recvd_buff;

int parse_string(char * input, int * output)
{
	// find newline
	char * p = strchr(input, '\n');
	char * p1 = strchr(input, '\n');   //pointer p1 will point to the first '\n'
	char * p2;
	
	if (!p)
	{
		printf("newline not found\n");
		return 0;
	}
	
	// find '^'
	p = strchr(p, '^');
	p2= strchr(p, '\n');     //this will point p2 to the second '\n'
	if (!p)
	{
		printf("'^' not found\n");
		return 0;
	}
	
//	p++;  // skip past character just found
	
	length=p2-p1;                                      \\legnth comes to be 37 correctly
	strncpy(recvd_buff, recvd_chars, length); \\ copy the string
	recvd_buff[99]='\0';                               \\ null terminate the string
	p = strchr(recvd_buff, '\n');                   \\ point the pointer to new array
                                                                   \\ original code from here..
	if (!p)
	{
		printf("newline not found\n");
		return 0;
	}
	// find '^'
	p = strchr(p, '^');
	if (!p)
	{
		printf("'^' not found\n");
		return 0;
	}
	
	p++;  // skip past character just found// variables used by scanf
	int n;
	char ch;
	int count = 0;
	
	// output extracted to array here
	int index = 0;
	
	while ((sscanf(p, " %d%c%n", &n, &ch, &count) > 1))
	{
		p += count;
		output[index++] = n;
	}
	
	return index;
}

[Chervil]

Wow. Sorry I can't make much sense of this - attempted to drop the above function parse_string() into my existing program in order to compile and test it.

Lines 2 and 3 don't mean anything, a couple of global variables pointing to some other global variables, which don't actually exist.

Multiple compiler errors "\\" used which is interpreted as am escape sequence representing a single backslash. I think you meant "//" (forward slash). The implication is this isn't the actual code you were running, but something you have edited in a text editor but not actually compiled or run.

More important errors:
line 29: length is not defined
line 30, neither recvd_buff nor recvd_chars exist.
line 31, recvd_buff[99]='\0';
why 99? seems to be just some arbitrary value pulled from thin air. How about using known information such as the value of length

In any case, the source of the copy should be one of your pointers (p, p1, p2), the destination should be a an unused buffer area, I've no idea what your, recvd_buff is, for all I know it might be the same as the input, I've no idea.

Line 32,

p = strchr(recvd_buff, '\n');                   \\ point the pointer to new array

As far as I can tell, this line set pointer p to the newline at the end of the data, thus there is no output at all.

What I suggest is this. Keep your functions simple, and self-contained. Don't have them relying on global variables lying around somewhere. Either declare local variables, or use the function parameters.

In terms of simplicity, rather than trying to patch new behaviour into existing code, consider breaking the task into smaller pieces and making the pieces into separate functions.

In this case I'm suggesting make the code to extract the substring between two newlines into a separate function.

[Chervil]

However - as I suggested previously, all of this is getting overcomplicated.

Quote:

either check for the final '!' - if that is guaranteed to always be there. If not, then check whether or not the next character pointed to by p is a newline '\n' ;

http://www.cplusplus.com/forum/general/183227/#msg897238

Testing the next character pointed to by p would have required just two lines of code.

[aliyesami]

sorry I didnt put in the complete code , its compiling and running .
can you review again ? the "\\" that you see is put directly in the code here as you see they are not in the code.

following your advise I wrote this function to extract the substring , does it look ok ?

int split_string(char * input, char * output) 
{
  int length=0;
  char * p = strchr(input, '\n');    // p will point to first '\n'
  char * p1,p2;
  p1=p ;                                    // save first '\n' position in p1
  p2 = strchr(p, '\n');                 // p2 will point to the next '\n'
  length = p2-p1;                       // length of the substring
  strncpy(output,p,length);          // copy the substring of size=length  starting 
                                                // from first '\n' to output string.
  return length;
}

[aliyesami]

but its giving me error on line 7 and 8 . same code is used in parse_string() and it goes through?

line 6 error: assignment makes integer from pointer without cast
line 7 error: invalid operands to binary - have 'int' and 'char'

[Chervil]

The error is on line 5:
char * p1,p2;
That is the same as:

char * p1;  // p1 is a pointer to char
char p2;    // p2 is not a pointer, its just a char 

My personal preference is, where appropriate, to avoid declaring a variable without any value, then later assigning a value.

Let me show what I mean:

Instead of

  char * p1;
  char * p2;
  p1=p ;                                    
  p2 = strchr(p, '\n'); 

I would prefer:

  char * p1 = p ;
  char * p2 = strchr(p, '\n'); 

Perhaps I could explain, but not in technical terms. A variable declared with no value, like this:
char * p1;
leaves me with an uneasy feeling, like I've forgotten something. Then suddenly I arrive somewhere important and realise I forgot to get dressed. So to avoid that uneasy nagging feeling, I try to assign a sensible value as soon as possible. - though in some circumstances it may not be possible. :)

OK, more seriously, your latest code for function split_string() is very close, it may need a little bit of slight tweaking, but it is nearly there.

[aliyesami]

thanks for valuable advise ,i modified the code further but now its only printing zeros and the size of array is also returned as zero.
did a little debugging and the split_sting() function is failing even before the strncpy().
i have put comments on each line and i am not seeing where i am going wrong

int split_string(char * input, char * output);
int parse_string(char * input, int * output);
char recvd_chars[100],recvd_buff[100],cmdbuff[100];
int size=0;

int main(void)
{
int size = split_string(recvd_chars,recvd_buff); 

}

int split_string(char * input, char * output)
{
	int length=0;
	char * p = strchr(input, '\n');    // p will point to first '\n'
	char * p1=p;                       // p1 will also point to first '\n'
	char * p2=strchr(p,'\n');          // p2 will point to the second '\n' 
	length = p2-p1;                    // length of the substring
	strncpy(output,p,length);          // copy the substring of size=length  starting
	// from first '\n' to output string.
	output[length+1]='\0';			   // null terminate the output string
	return length;
}

[Chervil]

Well, there are two problems.
The one you already noticed is that the length is zero. Let's consider why that is. If the length is zero, it means that p2-p1 == 0, Simple rearrangement of that expression leads to the conclusion that p1 == p2.

The two pointers are each pointing to the same character. Both are pointing to the first '\n'. How to fix that? Start the search for the second '\n' from the next character.

The other problem is that there is no terminating null character inserted in the output string after the last of the copied characters.

[aliyesami]

hi Chervil
isnt liine 21 null terminating the output ?

output[length+1]='\0';		

[Chervil]

isnt liine 21 null terminating the output ?

My apologies. It seems I was thinking of some earlier version of the code and wasn't reading properly. Yes, you are right.

(or maybe the post was edited while I was typing - doesn't matter anyway).

[aliyesami]

so i got one problem out of the way .. working on the first issue
thanks a lot for your patience and time

[aliyesami]

oh i think i figured the problem , for some stupid reason i was thinking the pointer will increment after the first initialization . I am now advancing the pointer and then looking for the second '\n'.
i am not home so I will test it in the evening and let you know.

int split_string(char * input, char * output)
{
	int length=0;
	char * p = strchr(input, '\n');    // p will point to first '\n'
	char * p1=p;                       // p1 will also point to first '\n'
        p++;                               // advance the p1 pointer past the first '\n'
	char * p2=strchr(p,'\n');          // p2 will point to the second '\n' 
        p= strchr(input, '\n');            // reinitialize the pointer p to point to first '\n'
	length = p2-p1;                    // length of the substring
	strncpy(output,p,length);          // copy the substring of size=length  starting
	// from first '\n' to output string.
	output[length+1]='\0';			   // null terminate the output string
	return length;
}

[aliyesami]

ok finally working , but I am not sure why I had to use length+1 ?

int split_string(char * input, char * output)
{
	int length=0;
	char * p = strchr(input, '\n');    // p will point to first '\n'
	char * p1=p;                       // p1 will also point to first '\n'
	p++;                               // advance the p1 pointer past the first '\n'
	char * p2=strchr(p,'\n');          // p2 will point to the second '\n'
	p= strchr(input, '\n');            // reinitialize the pointer p to point to first '\n'
	length = p2-p1;                    // length of the substring
	strncpy(output,p,length+1);          // copy the substring of size=length  starting
	// from first '\n' to output string.
	output[length+2]='\0';			   // null terminate the output string
	return length+1;
}

[Chervil]

not sure why I had to use length+1

I think this is just a matter of there being a newline at both ends, related to the the 'fence-post problem'.
https://en.wikipedia.org/wiki/Off-by-one_error#Fencepost_error

Take for example the sequence:
5678
We can see there are four digits.
But if we subtract the first from the last we get (8 - 5) which gives 3, not 4

One other comment, I think your null terminator is off by one position too.
I think it should be:

    output[length+1]='\0';             // null terminate the output string 

Most likely the reason for this discrepancy is that you forgot that subscripts begin from 0, not 1.

[aliyesami]

thanks a lot Chervil everything is working fine now. I learned a lot from this exercise.
appreciate your help and time