Thread: Review and criticism appreciated!

Was writing a lexer(lexical analyzer) function and well I hadn't messed with dynamic memory allocation before was like the last thing left in my k&r book.
So anyway criticize away! Just want to figure out how I'm doing so far with teaching myself C.

Code:

int lexer(char s[], int *ptr, char **ptrarray)
{
	int i, w=0, n;
	int rows=0, cols=0;
	char *buffptr;
	int *temp;
	
	temp = ptr;
	while(*temp != 0)
	{
		rows++;
		i = *temp;
		if(i>cols)
			cols=i;
		++temp;
	}
	
	for(i=0; i<rows; i++)
		if( (ptrarray[i] = malloc((cols+1) * sizeof(char *))) == NULL)
		{
			printf("Memory Allocation Failed\n");
			while(i>0)
			{
				free(ptrarray[i]);
				i--;
			}
			exit(-1);
		}
	
	buffptr = s;
	while( (n = *ptr) != 0 && n <2048)
	{
		strcpy(ptrarray[w], buffptr);
		
		for(i=0; i<n; i++)
			++buffptr;

		++ptr;
		w++;
	}
	return rows;
}

**Explanation of function**
Basically takes a string and an int array where each element of the int array is loaded with different lengths of each token you want to break apart a string. I used a pointer array to set a pointer to the beginning of each token in the string.

Quick and dirty example use..

Code:

#include <string.h>
#include <stdio.h>
#include <stdlib.h>

int lexer(char s[], int *ptr, char **ptrarray);

int main(void)
{
	char buffer[200] = "This is my string test yeah";
	char testbuff[200];
	char **ptrarray;
	char *temp;
	int array[20];
	int i, w, rows;

	for(i=0; i<20; i++)
		array[i] = 0;

	array[0] = 5;
	array[1] = 3;
	array[2] = 3;
	array[3] = 7;
	array[4] = 5;
	array[5] = 4;


	rows = lexer(buffer, array, ptrarray);

	w=0;
	while(array[w] != 0)
	{
		temp = ptrarray[w];
		for(i=0; i<array[w]; i++)

		{
			testbuff[i] = *temp;
			++temp;
		}
		testbuff[i++] = '\0';
		printf("%s\n", testbuff);
		w++;
	}

	for(i=0; i<rows; i++)
		free(ptrarray[i]);
	return 0;
}
int lexer(char s[], int *ptr, char **ptrarray)
{
	int i, w=0, n;
	int rows=0, cols=0;
	char *buffptr;
	int *temp;
	
	temp = ptr;
	while(*temp != 0)
	{
		rows++;
		i = *temp;
		if(i>cols)
			cols=i;
		++temp;
	}
	
	for(i=0; i<rows; i++)
		if( (ptrarray[i] = malloc((cols+1) * sizeof(char *))) == NULL)
		{
			printf("Memory Allocation Failed\n");
			while(i>0)
			{
				free(ptrarray[i]);
				i--;
			}
			exit(-1);
		}
	
	buffptr = s;
	while( (n = *ptr) != 0 && n <2048)
	{
		strcpy(ptrarray[w], buffptr);
		
		for(i=0; i<n; i++)
			++buffptr;

		++ptr;
		w++;
	}
	return rows;
}

I know one thing I need to get in the habit of is commenting my code while I write it, I usually end up going back through and commenting it... bleh.

Originally Posted by dwks

Code:

for(i=0; i<n; i++)
    ++buffptr;

->

Code:

buffptr += n;

Ah! Didn't think of that, thanks dwks

Originally Posted by dwks

Some of your while loops would probably be better as for loops.

Code:

while( (n = *ptr) != 0 && n <2048)

Is that code right? Or should it involve ptr somehow?

That was some old code that slipped through my redesign I'll take out the

&& n<2048

Code:

#include <string.h>
#include <stdio.h>
#include <stdlib.h>

int lexer(char s[], int *ptr, char **ptrarray);

int main(void)
{
	char buffer[200] = "This is my string test yeah";
	char testbuff[200];
	char **ptrarray;
	char *temp;
	int array[20];
	int i, w, rows;

	for(i=0; i<20; i++)
		array[i] = 0;

	array[0] = 5;
	array[1] = 3;
	array[2] = 3;
	array[3] = 7;
	array[4] = 5;
	array[5] = 4;


	rows = lexer(buffer, array, ptrarray);


....

I know this is just an example you threw together, but its possible to change all the initialization you have for "array[20]" to simply: int array[20] = {5,3,3,7,5,4,0} (will set the rest of the elements to 0.).

I know you can load an array like that but didn't know it would fill in the remaining elements of the array with the last you specify.
Thanks

Thanks for all the criticisms so far

Originally Posted by Sysop_fb

I know you can load an array like that but didn't know it would fill in the remaining elements of the array with the last you specify.
Thanks

Thanks for all the criticisms so far

Yeah, in fact you dont even need that extra "0" i put in the last element, it will fill the rest of the array with zeros if you only initialize one element, I just put it there for clearence.

Cool

Also is this a good thing to do when mallocing an array of pointers? If malloc hits NULL after mallocing some already then freeing the ones you already malloced?

Code:

	for(i=0; i<rows; i++)
		if( (ptrarray[i] = malloc((cols+1) * sizeof(char *))) == NULL)
		{
			printf("Memory Allocation Failed\n");
			while(i>-1)
			{
				free(ptrarray[i]);
				i--;
			}
			exit(-1);
		}

I also changed that from my original instead of while(i>0) while(i>-1) that way it doesn't miss freeing ptrarray[0].