OK, I see now why it is only cosmetic. It only ends up being that way
because the strings differ only in a character that is not a letter.
That is a cumbersome way to do case insensitive comparison. It would
be a lot easier to lose the third parameter and do:
if (arg1.size() != arg2.size() ) return false;
for(int i = 0; i < arg1.size(); i++) {
if (apr_toupper( arg1[i] ) != apr_toupper( arg2[i] ) ) return false;
}
(using apr_toupper for maximum portability). This could have problems
if you were trying to compare against accented or foreign characters,
but I think you are only comparing against ASCII.
On 6/6/08, Curt Arnold <[EMAIL PROTECTED]> wrote:
>
> On Jun 5, 2008, at 11:10 PM, Dale King wrote:
>>
>>
>> I saw that when you made the checkin, but I don't understand why you
>> think it is only a cosmetic bug. It seems to me that if you were
>> trying to configure a syslog appender for LOCAL2 for instance it would
>> call setFacility with LOCAL2 as the facility name. This would call
>> getFacility( "LOCAL2" ) which will not find a mapping for "LOCAL2" and
>> instead of returning the value LOG_LOCAL2 as it should it will return
>> LOG_UNDEF. This will cause the facility to default to USER.
>>
>> I don't know anything about syslog to know what the ramifications of
>> that are, but I don't see how you can claim this behavior is merely
>> cosmetic.
>
> The code fragment under question is:
>
> else if (StringHelper::equalsIgnoreCase(s,
> LOG4CXX_STR("LOCAL0"), LOG4CXX_STR("local0")))
> {
> return LOG_LOCAL0;
> }
> else if (StringHelper::equalsIgnoreCase(s,
> LOG4CXX_STR("LOCAL1"), LOG4CXX_STR("local1")))
> {
> return LOG_LOCAL1;
> }
> else if (StringHelper::equalsIgnoreCase(s,
> LOG4CXX_STR("LOCAL1"), LOG4CXX_STR("local2")))
> {
> return LOG_LOCAL2;
> }
> else if (StringHelper::equalsIgnoreCase(s,
> LOG4CXX_STR("LOCAL1"), LOG4CXX_STR("local3")))
> {
> ...
>
> Where the second argument of equalsIgnoreCase is "LOCAL1" where it
> wasn't intended to be.
>
> equalsIgnoreCase is essentially:
>
> if (arg1.size() != arg2.size() || arg1.size() != arg2) return false;
> for(int i = 0; i < arg1.size(); i++) {
> if (arg1[i] != arg2[i] && arg1[i] != arg3[i]) return false;
> }
> return true;
>
>
> if arg1 is "LOCAL1", "local1", "LoCaL1" or any other variant, it will
> return LOG_LOCAL1, since it will match the second else if. It will
> not fall through to the 3 else if or later that would incorrectly
> match since they have 1 as one of the 2 options for the 6 character.
>
> If arg2 is "LOCAL2", "local2", "LoCal2" or any other variant, it will
> return LOG_LOCAL2. equalsIgnoreCase only has to have a match on one
> of the two potential characters and so as long as one of the matching
> strings has the right number, it will catch all the expected matches.
> All of the incorrect matches would already have been eliminated by the
> second else if.
>
> Basically, the misplaced "LOCAL1" adds the potential for false matches
> in the later clauses, but any potential false match has already been
> matched.
>
--
Dale King
