c - find in array function question.

[cyclone3d]

I am working on some debugging of pktriggercord and have come across a function that seems to have some flawed logic in it.. either that or I am for some reason not seeing why this function is written how it is written.

String compare function:

// case insenstive comparison
// strnicmp
int str_comparison_i (const char *s1, const char *s2, int n) {
    if( s1 == NULL ) {
	return s2 == NULL ? 0 : -(*s2);
    }
    if (s2 == NULL) {
	return *s1;
    }

    char c1='\0', c2='\0';
    int length=0;
    while( length<n && (c1 = tolower (*s1)) == (c2 = tolower (*s2))) {
	if (*s1 == '\0') break;
	++s1; 
	++s2;
	++length;
    }
    return c1 - c2;
}

find in array function:

int find_in_array( const char** an_array, int length, char* str ) {
    int i;
    int found_index=-1;
	size_t found_index_length = 0;
	size_t string_length;
    for( i = 0; i<length; ++i ) {
		string_length = strlen(an_array[i]);
	if( (str_comparison_i( an_array[i], str, string_length ) == 0) && (string_length > found_index_length) ) {
	    found_index_length = string_length;
	    found_index = i;
	}
    }
    return found_index;
}

My question is this:
Looking at (string_length > found_index_length), what does it matter if this is even in there?
Is there any point to even checking to see if the string length in the current array index is greater than the string length in the previous array index?

edit: I see what it is doing. It is going through the whole array, looking for the array index where the string most matches the string being searched for.

1. If it finds a partial match, it marks that index
2. If it finds another partial match, it then compares the length of that match to the previous match. If the current match is longer than the previous match, it changes the index marked.

 

[mikeblas]

str_comparison_i() seems kind a broken; what if both s1 and s2 are NULL? What if s2 runs out of characters before s1?

Why is the third parameter to find_in_array not const?

Why call strlen() on every string in the loop of find_in_array()? It's expensive, but not necessary.

This code will crash, though, because of the str_comparison_i() problems.

 

[aL Mac]

str_comparison_i() seems kind a broken; what if both s1 and s2 are NULL? What if s2 runs out of characters before s1?

Why is the third parameter to find_in_array not const?

Why call strlen() on every string in the loop of find_in_array()? It's expensive, but not necessary.

This code will crash, though, because of the str_comparison_i() problems.

What do you mean what if both s1 and s2 are NULL? It handles that case and returns 0.

As far as s2 running out of chars before s1, it looks to me that the caller is required to pass in n such that it is less than or equal to the length of both s1 and s2. I wonder if there is documentation on this function. They could check for a null termination and quit though to be more safe, especially since they are doing it for s1. If they don't do it at all neither check should be in there and it should be required to pass in n less than or equal to the length of s1 or s2. If they put the check in there, it should probably be for both strings.

 

[mikeblas]

Oops, you're right, s1 == s2 == NULL is handled.

Indeed, it could be that the caller is required to pass in the minimum of the length of s1 or s2 as the n parameter. Thing is, the one call site we can see doesn't do that; it always passes in the length of s1, never s2. The caller of find_in_array() can't reasonably be expected to know that s2 always has more characters than any string in the an_array array. If the loop in find_in_array() got the length of its str parameter just once, outside the loop, it wouldn't call strlen() on each element in the array and would be more resilient to short strings, I think.

 

[cyclone3d]

str_comparison_i() seems kind a broken; what if both s1 and s2 are NULL? What if s2 runs out of characters before s1?

Why is the third parameter to find_in_array not const?

Why call strlen() on every string in the loop of find_in_array()? It's expensive, but not necessary.

This code will crash, though, because of the str_comparison_i() problems.

And this is after I replaced strlen(an_array) everywhere the "string_length" variable is.
See 2. in my original response. This is why it checks the length each loop itteration.

What do you mean what if both s1 and s2 are NULL? It handles that case and returns 0.

As far as s2 running out of chars before s1, it looks to me that the caller is required to pass in n such that it is less than or equal to the length of both s1 and s2. I wonder if there is documentation on this function. They could check for a null termination and quit though to be more safe, especially since they are doing it for s1. If they don't do it at all neither check should be in there and it should be required to pass in n less than or equal to the length of s1 or s2. If they put the check in there, it should probably be for both strings.

Yeah, the code is quite buggy. There is almost no documantation anywhere in the code. Would be so much easier if there was. I am adding as I figure stuff out.

Oops, you're right, s1 == s2 == NULL is handled.

Indeed, it could be that the caller is required to pass in the minimum of the length of s1 or s2 as the n parameter. Thing is, the one call site we can see doesn't do that; it always passes in the length of s1, never s2. The caller of find_in_array() can't reasonably be expected to know that s2 always has more characters than any string in the an_array array. If the loop in find_in_array() got the length of its str parameter just once, outside the loop, it wouldn't call strlen() on each element in the array and would be more resilient to short strings, I think.

This could very well be one reason why the program is kind of buggy. I will do some more looking to see why it is setup this way and probably rewrite it to work better.

 

[Zachafer]

#include <strings.h>    //same thing as include string.h

bool cmpIgnoreCase( const string& s1, const string& s2 ) {
    return strcasecmp( s1.c_str(), s2.c_str() ) <= 0;
}