202543 - Useless assertion in mozilla/dom/src/base/nsJSEnvironment.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[tenthumbs]

Making a wild guess about component.

Gcc issues this warning:

nsJSEnvironment.cpp:919: warning: comparison is always true due to limited range
of data type

Source is:

     907 void
     908 AtomToEventHandlerName(nsIAtom *aName, char *charName, PRUint32
charNameSize)
     909 {
     910   // optimized to avoid ns*Str*.h explicit/implicit copying and malloc'ing
     911   // even nsCAutoString may call an Append that copy-constructs an
nsStr from
     912   // a const PRUnichar*
     913   const char *name;
     914   aName->GetUTF8String(&name);
     915   char c;
     916   PRUint32 i = 0;
     917 
     918   do {
     919     NS_ASSERTION(name[i] < 128, "non-ASCII event handler name");

On Linux, chars are signed by default so gcc knows that they can only be in the
range [-128,127]. Thus the warning.

If you really want a test here, then you want something like:
  name[i] & 0x80 != 0
which should work for signed and unsigned chars.

Comment 1

[Johnny Stenback (:jst)]

Sounds good to me, you wanna whip up a patch that does that? :-)

Comment 2

[tenthumbs]

Attachment: patch

Since you asked so nicely. :-)

I moved the assertion after the assignment. It seemed like a good idea.

BTW, the nest assertion uses isalpha. Is that a good idea? Also the assertion
after that tests for buffer overrun but there's no run-time test. Shouldn't
there be?

Comment 3

[Johnny Stenback (:jst)]

Thanks for the patch! Re: the bounds checking, that could be removed all
together, now that nsIAtom's hold char* data, you could simply make
AtomToEventHandlerName() return a "const char *" and remove the static buffers
alltogether. You could leave the loop in there to assert that all characters are
alpabetic (using isalpha), but put the loop inside #ifdef DEBUG ... #endif.
Also, there's no need for two assertions in that loop, isalpha() alone should be
enough, and '\0' is not an alphabetic character, so no need to check that either.

tenthumbs, wanna have a stab at taking this a bit further, and reducing our
codesize and speeding this code up a bit?

Comment 4

[tenthumbs]

Attachment: patch

Why not as long as you realize I know nothing about the dom code you this 
is all hand waving. :-)

I don't like isalpha because it's locale dependent. This is a shared 
library so you never know what someone else may have done. Throwing bogus 
assertions, or not throwing valid ones, doesn't seem worth the trouble. 
Besides, the test you want is simple.

AtomToEventHandlerName is now very simple in an optimized build. It's also 
just an internal function so I made it static inline. That should do nice 
things for speed and size, nut not in a debug build of course.

I gave this patch a quick test and nothing nasty happened but that doesn't 
mean much. Give it a lot of testing.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 121675 [details] [diff] [review]
patch

Perfect! sr=jst

Comment 6

[Johnny Stenback (:jst)]

Fix checked in!