225887 - trivial optimization in nsCharTraits.h

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[[e-mail]]

http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsCharTraits.h#175

there's no need for two |if| tests... we can just return the difference if the
two chars aren't equal. we use memcmp elsewhere in nsCharTraits, whose return
values aren't restricted to {-1,0,1}... so this optimization is unlikely to
cause a problem.

Comment 1

[[e-mail]]

Attachment: fix

Comment 2

[Alec Flett]

Comment on attachment 135653 [details] [diff] [review]
fix

yay! less branching.

sr=alecf

Comment 3

[Alec Flett]

Comment on attachment 135653 [details] [diff] [review]
fix

yay! less branching.

sr=alecf

Comment 4

[jag (Peter Annema)]

Comment on attachment 135653 [details] [diff] [review]
fix

Why not simply:

#else
return to_int_type(*s1 - *s2);
#endif

Comment 5

[[e-mail]]

we want to compare the whole string, right?

(sorry about lack of context in the diff, see lxr link in comment 0)

Comment 6

[jag (Peter Annema)]

Comment on attachment 135653 [details] [diff] [review]
fix

Duh, the for loop was enough context. pebcak (it was too early in the morning
to review). r=jag

Comment 7

[[e-mail]]

np :)

fixed.

Comment 8

[Brian Ryner (not reading)]

Whoa, this isn't right.  to_int_type does an intermediate cast to PRUint16, so
the resulting code is similar to:

return int((unsigned short)(*s1 - *s2));

which returns 65535 for *s1 = 'a', *s2 = 'b'.

This broke sorting in the Linux filepicker and probably other places.

Comment 9

[[e-mail]]

right - i actually suspected that might be a problem, and tested it before i
landed. my testing showed it was fine, so maybe i made a mistake when testing...
sigh :/

thanks for the report. patch coming up.

Comment 10

[[e-mail]]

Attachment: le fix

Comment 11

[[e-mail]]

Comment on attachment 135987 [details] [diff] [review]
le fix

this look ok, alecf?

Comment 12

[Alec Flett]

Comment on attachment 135987 [details] [diff] [review]
le fix

oh, duh :)
sr=alecf

Comment 13

[[e-mail]]

*** Bug 226310 has been marked as a duplicate of this bug. ***

Comment 14

[[e-mail]]

checked in.