Prog Hangs - Something To Do With Pointers

I'm making a substring function. When I call it the prog hangs and I don't understand why Heres my function:

Code:

int String::Sub(char *sub, bool case_sensitive=true)
{
	//Searches for a substring within the string
	//Returns -1 for failure or the index of first occurence
	bool in=false;
	char *a='\0', *b='\0';
	int found=-2; //if returns -2 then somethings wrong

	for(int i=0; i<size; i++)
	{
		a=&str_ptr[i];	
		if(!case_sensitive)
			if(*a >= 'A' && *a <= 'Z') 
				*a+=32;
		
		if(!in)
		{					
			b=sub;
			if(!case_sensitive)
				if(*b >= 'A' && *b <= 'Z') 
					*b+=32;

			if(*a == *b)
			{
				found=i;
				in=true;
			}			
		}
		else
		{
			b++;
			if(*b == '\0') return found;
			
			if(!case_sensitive)
				if(*b >= 'A' && *b <= 'Z') 
					*b+=32;

			if(*a != *b) 
			{	
				in=false;
				i=found+1;
			}
		}
	}
	return -1;
}

If I comment out the red section it works fine, but it won't do a non case sensitive comparison properly. The thing is it works fine for all the other instances where I convert to lower case. Also heres my main function with headers and stuff excluded:

Code:

int _tmain(int argc, _TCHAR* argv[])
{
	String something("Something\n");
	something.Print();
	something.Add("Else\n");
	cout << something.Sub("ELSE", false);
	cin.ignore();
	return 0;
}

Anyone know why this is happening?

I don't know why your code isn't working right. Although this bit looks a bit suspicious:

Code:

i=found+1;

. If you are unlucky, you'll keep going over the same bit of string infinitely.

Some other comments:
I would recommend that you don't change the actual case of the input string. Instead, extract a character from the string, then fold the character to lowercase (if relevant). Also, use "tolower()" - it's more portable and may even work for foreign character sets.

Similarly, it is quite unnecssary to take the address of the current character into a pointer a, when all you really need is a copy of the char str_ptr[i].

There's quite a bit of duplication between your if/else branches - it is always best to avoid duplicating code.

--
Mats

Oh yeah I'm an idiot. I forgot that using pointers is going to change the contents o_0
I guess I'll start over. I think the i=found+1 but should never get stuck, because its always moving forward, but it might not be the best way to do this.

Cheers.

>> char *a='\0';

that initialization seems a bit confused...

>> *b+=32;

you are modifying the callers string, which happens to be, in this case, a const literal. write the character to a temporary before comparison.

Originally Posted by Sebastiani

>> char *a='\0';

Nah, that's just a different way of saying

Code:

a = NULL;

- ok, so it may not be perfectly compatible types, but ignoring warnings, it's fine. - not very good style, tho'.

--
Mats

Okay I got it working now and im using tolower() as that should be better.

>> char *a='\0';

that initialization seems a bit confused...

Thats the only way I know that I can get it declared and initialized in one line. And I don't get any warnings from it.

Originally Posted by mike_g

Thats the only way I know that I can get it declared and initialized in one line. And I don't get any warnings from it.

What compiler are you using, and what warning levels do you have enabled? It is definitely not "right" to do that. Try something like:

Code:

   char *a = 0;

But then you don't really need char *a if you follow the advice about using a temporary variable. You can just do something like:

Code:

   char a;
   ...
   a = str_ptr[i];
   ...
   if (a == *b)
     ...

Another point: Add braces at least for your if-statements that have more than one line inside them - it prevents you from getting strange effects when you decide to add a debug statement, such as:

Code:

   if (something) 
       cout << "something is true";
       do some stuff;

--
Mats

What compiler are you using, and what warning levels do you have enabled? It is definitely not "right" to do that. Try something like:
Code:

char *a = 0;

I get no warnings with that either. Is it okay to do that instead? i'm using VC++2005 with default warining levels. Maybe I'll see if I can turn it up.

Another point: Add braces at least for your if-statements that have more than one line inside them - it prevents you from getting strange effects when you decide to add a debug statement, such as:

Yeah I know, its a bad coding habit of mine.

Oh and you were right about the i=found+1 thing being wrong, because I forgot i gets incermented at the start of each loop. But I still don't reckon it can get stuck in the loop.

Cheers.

Originally Posted by mike_g

Oh and you were right about the i=found+1 thing being wrong, because I forgot i gets incermented at the start of each loop. But I still don't reckon it can get stuck in the loop.

Cheers.

Yes, I guess you are right there.

--
Mats