Thread: browsing through history commands

for 17 commands you need array of 17 members, not 16

And 2 indexes - last stored command and fisrt stored command that will be advanced in the cyclic manner.

The rest that you described seems to be OK

Keep an index counter. When the user types something, move it over to p[index]. Then increment index. If it goes beyond the limit, set index to 0. That way you have a circular array, and you're always pointing to the latest entry typed in.

fgets is inclusive of the null terminator, so its,

Code:

fgets(p[pindex], 128, stdin);

Also don't cast malloc see the FAQ.

Not sure what you mean, but

Code:

if(pindex > 15) {
pindex = 0;
}

Isn't inclusive of 15, so 16 is a valid index (in-fact its not, its out-of-bounds)

Code:

if(pindex >= 15)
    pindex = 0;

You also don't check if malloc fails (it returns NULL if it fails).

You have a memory leak if the user types more than 16 things. First of all, set every element of p to zero at the start. Then before you malloc(), check to see if p[pindex] is NULL. If it is, only then do you have to malloc().

To adjust pindex back to 15 for the last element, I would recommend an extra variable to calculate the last index.

Here's an altered version of your code:

Code:

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

int main(void)
{
	char *p[16] = { NULL };
	int pindex = 0, plast;
	
	while(1)
	{
		if(!p[pindex])
		{
			p[pindex] = (char *)malloc(sizeof(char)*128);
			if(!p[pindex])
			{
				printf("Out of memory.\n");
				break;
			}
		}
		fgets(p[pindex], 128, stdin);
		/* if(!strcmp(p[pindex], "break")) break; */
		plast = (pindex == 0) ? (15) : (pindex - 1);
		if((pindex == 0) && (!p[15]))
		{
			printf("There is no last item\n");
		}
		else
		{
			printf("Last thing you typed: &#37;s\n", p[plast]);
		}
		pindex = (pindex + 1) % 16;
	}
	for(pindex = 0;pindex < 16;pindex++)
	{
		free(p[pindex]);
	}

	return 0;
}

Originally Posted by apsync

Then, I get an error saying could not convert from void *, I guess its my compiler..

Remember to include stdlib.h. If that doesn't fix it, you might be compiling this as a C++ program. C++ doesn't allow implicit casts from void *. C does.

Originally Posted by apsync

Thanks, I think I can sort it out from here, however, I do not understand this part,

Code:

pindex = (pindex + 1) % 16;

what is the purpose of this?

pindex will be raised by one, and then integer divided by 16 and the remainder will be assigned to pindex.

If pindex is 0, pindex + 1 is 1, and then integer divided by 16 is 0, with a remainder of 1. Hence pindex is just raised to one.

The trick is for when pindex is 15. pindex + 1 would be 16. 16 / 16 is 1, with a remainder of 0.

That's how you can get pindex to loop back to 0.

to avoid checking null pointer on every iteration of the loop - you can allocate memory for all 16 strings before loop, check that all pointers are not null and then - run the loop already without check of the null-pointer...

And free the memory after the work is done - as you do now...