165877 - ns[C]String::ToFloat broken. Will never report bad number.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Bratell]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1b) Gecko/20020317
Build Identifier: 

nsString::ToFloat is broken. It will never report a bad number. See this part of
function:

  if (*cp != 0) {
    *aErrorCode = (PRInt32) NS_ERROR_ILLEGAL_VALUE;
  }
  *aErrorCode = (PRInt32) NS_OK;
  return f;

obviously aErrorCode is always NS_OK.


I looked at from a performace view though. Why are we using our own PR_strtod
anyway?

Reproducible: Always

Steps to Reproduce:

Comment 1

[Daniel Bratell]

That goes for both nsString and nsCString.

Comment 2

[Daniel Bratell]

Attachment: Rewrite of ToFloat

This is a rewrite of the functions that do report the error. It also uses
strtod instead of PR_strtod since strtod is faster. PR_strtod had some locking
overhead that I thing is the reason it was slow. When measured in Quantify the
change was a speedup from 26 us/call to 18 us/call  (us=microseconds).

I'm looking for r= and sr= on this.

Comment 3

[Daniel Bratell]

Taking the bug.

Comment 4

[jag (Peter Annema)]

It looks like the old ToFloat code doesn't free the copies it makes. In your new
code, you don't free the result of ToCString.

I don't know why we're using PR_strtod instead of strtod. The only reason I can
think of is that at the time strtod didn't do everywhere what it should, or it
gives different results per platform when we want the same result everywhere.
scc (e-mail him) and dougt might know more about this.

Comment 5

[Daniel Bratell]

ToCString doesn't allocate. It uses the buffer passed to it so there is no
memory leak since we pass a stack buffer. 

I've tried to see why we use PR_strtod and as far as I can tell it's just
because we always have. The PR_strtod is one of David Gay's routines from 1991
and I think it was adopted to run in other environments then Mozilla use today.
Also, we already use the C library's strtod in libpng without any known problems
to me. But I too would like to hear what scc has to say about it.

Comment 6

[jag (Peter Annema)]

Heh, I knew I was missing something, it just wouldn't have made sense if that
old code was leaky.

Comment 7

[Alec Flett]

Comment on attachment 97383 [details] [diff] [review]
Rewrite of ToFloat

hmm.. is strtod() portable and reliable on all platforms? I'm not sure the
value I see in switching away from PR_strtod()

Comment 8

[Daniel Bratell]

See comment 2 about the reason to use strtod instead of PR_strtod. The function
strtod is in ANSI C and should be available everywhere. Maybe it was missing 10
years ago on some platform Netscape considered important and that's why it's in
NSPR, but I see no reason to use PR_strtod today.

Comment 9

[jag (Peter Annema)]

Note that libpng has a strtod wrapper around wcstod for WinCE:

http://lxr.mozilla.org/seamonkey/source/modules/libimg/png/pngrutil.c#17

Comment 10

[Alec Flett]

Comment on attachment 97383 [details] [diff] [review]
Rewrite of ToFloat

I'll bet wince is a unique case, since I'll bet many of the platforms it runs
on (ARM, etc) don't have floating point units.

Comment 11

[Daniel Bratell]

Does that mean that I can get a positive review of the code? jag? alecf?

Comment 12

[Alec Flett]

Comment on attachment 97383 [details] [diff] [review]
Rewrite of ToFloat

sr=alecf

Comment 13

[Daniel Bratell]

Fix checked in

Comment 14

[Boris Zbarsky [:bzbarsky]]

This caused bug 183211; not quite sure why yet.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem is that you moved to using a locale-sensitive libc function.  (And
given that it's locale-sensitive, I doubt that it's faster.  Did you measure?)

Comment 16

[Daniel Bratell]

Yes, I measured, see comment #2. Then again, strtod on Windows (where I do my
tests) doesn't seem to be sensitive to locale so the performance situation may
be different on other platforms. 

Thanks for the quick fix!