Problem using an array inside a switch statement

Question

I am having a problem when using a switch statement to check for something inside an array. I am using an int as my switch, then I compare the character arrays using strstr. So it reads like this:

void function(char *buffer, uint32_t size){
 char *p=(char*)malloc(compareSize); //compareSize is the size of the text to be compared
 #define TXTCOMPARE1 (char*)"Sample Text 1"
 #define TXTCOMPARE2 (char*)"Sample Text 2"
 #define TXTCOMPARE3 (char*)"Sample Text 3"
 switch(intCompare){
 case COMPARE1 :
 p = strstr(buffer,TXTCOMPARE1); break;
 case COMPARE2 :
 p = strstr(buffer,TXTCOMPARE2); break;
 case COMPARE3 :
 p = strstr(buffer,TXTCOMPARE3); break;
 }
 if(p!=NULL) { Serial.println("Success!"); }
 else { Serial.println("No match"); }
 memset(&p,0,sizeof(p);
}

This method does not work and p is always NULL. However, when I write it out without the switch() it compares just fine:

//using the same p declaration as above
p = strstr(buffer,TXTCOMPARE1);
if(p != NULL) { Serial.print("Success:"); }
else { Serial.print("No Match"); }

Why would the switch statement give me different results for what is basically the same function call?

Edit: Actually it seems to break everywhere when I set more than one call to p = strstr(...). When there is only one strstr it works, when two or more are used, it does not...

• Kugan Kumar: Have you defined COMPARE1, COMPARE2 and COMPARE3 ? Where is your value assigning statement for intCompare ?
• Andrew: Yes, I appologize for leaving that out. But that is almost exactly what I'm doing, except that intCompare is being passed into function(char *buffer, uint32_t size, int intCompare). But when I have multiple cases, p is always NULL
• John Burger: I don't understand three things in this code: 1) Why are you doing a malloc() into p, and then overwriting p before doing a free()? That is not what strstr() needs to work. 2) Why do the TXTCOMPARE strings have (char *) before them? This shouldn't be necessary. 3) Why are you doing a memset(&p,0,sizeof(p));? This is an extraordinarily long-winded way of saying p = NULL; - and then p goes out of scope anyway...
• Andrew: Thanks @JohnBurger. 1) I'm trying to put in an expected size for p from the comparison text I am expecting. 2) The compiler was giving me warnings for converting the strings into char* when defining them, so I cast them as char*. Is there a better way to do this? 3)I thought I was trying to clear the p pointer.

Answer 1

One big problem is that you forgot the break statements for those cases. So if it compares in the first one and finds it, you'll still get null because it is going to go look for the third one afterwards.

Answer 2

I solved this problem, there were a few problems with the code that I found, so thanks for your comments. Basically, it lies in the #define# statements. First, in Arduino it's not such great practice to use #define for strings/chars, only for int. This is because the compiler applies the text every time they appear in the code, so it uses up a lot more memory. The problem here is solved by using static char TXTCOMPARE1 = {"Sample Text 1"};

Another problem was that there was no NULL terminated character at the end of the #define statement, so the program kept looking to the end of the stack when using strstr() and finding no termination, I think I was just getting lucky that it worked occasionally at all.

Furthermore, there is no need to clear p using memset. It needs to be declared as NULL and then reset after the switch by if(p){p = NULL;}.

And now it works!

• John Burger: Good that you found it - and thank you for the caution about #define and strings. Note that you should have used static const char TXTCOMPARE1[] = "Sample Text 1";, and also that if (p) { p =NULL; } wastes an if. Just do p = NULL; - it's smaller and faster. Oh, and completely unnecessary. p is about to disappear anyway when it goes out of scope: no need to zero it out.
• Nick Gammon: Another problem was that there was no NULL terminated character at the end of the #define statement - that's not true. It's hard to tell from your snippet but a #define just does a textual replacement.
• Andrew: There is no need to malloc memory for p, just declare it as a char *p = NULL; strstr then returns a pointer to the existing strings. All the malloc line is doing is leaking memory.
• user31481: The statement "This is because the compiler applies the text every time they appear in the code, so it uses up a lot more memory" is wrong. No matter what declaration you use (#define, const char), the memory usage is the same. The C compiler take care of reusing the string when it appears for second time. Wrote a little test program and see how much RAM/program memory it use.