[llvm-commits] [llvm] r50821 - /llvm/trunk/include/llvm/ADT/StringExtras.h

[Bill Wendling]

On May 7, 2008, at 12:17 PM, Ted Kremenek wrote:
> On May 7, 2008, at 11:45 AM, Bill Wendling wrote:
>
>> On Wed, May 7, 2008 at 11:35 AM, Ted Kremenek <kremenek at apple.com>  
>> wrote:
>>> +/// CStrInCStrNoCase - Portable version of strcasestr.  Locates  
>>> the first
>>> +///  occurance of c-string 's1' in string 's2', ignoring case.   
>>> Returns
>>> +///  NULL if 's1' cannot be found.
>>> +static inline const char* CStrInCStrNoCase(const char *s1, const  
>>> char *s2) {
>>> +
>>> +  // Are either strings NULL?
>>> +  if (!s1 || !s2)
>>> +    return 0;
>>> +
>>
>> Should there be an early return for when s1 and s2 are the same  
>> value?
>
> Yes.  Good idea.
>>
>
Also, for parity with strcasestr, you might want to return s2 if s1 is  
the empty string.

>>> +  const char *I1=s1, *I2=s2;
>>> +
>>> +  while (*I1 != '\0' || *I2 != '\0' )
>>> +    if (tolower(*I1) != tolower(*I2)) { // No match.  Start over.
>>> +      ++s1; I1 = s1; I2 = s2;
>>
>> If you increment s1 here, then the function's comment is wrong. Do  
>> you
>> mean to increment s2 instead?
>
> Good catch.  I was thinking of strcasestr, which looks for s2 in s1,  
> not s1 in s2.  I need to fix this by incrementing s2 instead.

Would it be a good idea to have the parameters be in the same order as  
strcasestr to avoid confusion?

>>> +    }
>>> +    else { // Character match.  Advance to the next character.
>>> +      ++I1; ++I2;
>>> +    }
>>> +
>>> +  // If we exhausted all of the characters in 's2', then 's1'  
>>> does not occur
>>> +  // in it.
>>> +  return *I2 == '\0' ? 0 : I1;
>>
>> Is this correct? If we have something like:
>>
>> s1 = "bar"
>> s2 = "foobar"
>>
>> then I2 will be \0 here, but we still found s1 in there.
>
> Yeah this looks completely wrong.  What about (assuming we increment  
> s2 instead of s1):
>
>  return *I1 == '\0' ? s2 : 0;
>
This might work. You'll have to test it out to make sure. :-)

-bw