240106 - ns(C)String::AppendInt(PRInt64) would be nice

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

I have a patch for this; also cleans up AppendInt (to use PR_snprintf)

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[tenthumbs]

Beware of PR_snprintf, it's broken. See bug 162786, bug 186547, and bug 
229662. Despite the patch in bug 186547 it still returns a useless value and 
it still doesn't obey its own api.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm not interested in PR_snprintf's return value here.

Comment 4

[Brendan Eich [:brendan]]

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

> void
> nsCString::AppendInt( PRInt32 aInteger, PRInt32 aRadix )
>   {
>     char buf[] = {'0',0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};

Anyone know what's up with this?  Why is it important to zero-initialize?  And
why *shouldn't* the array be dimensioned explicitly?

/be

Comment 5

[tenthumbs]

> I'm not interested in PR_snprintf's return value here.

But, as Brendan points out, you haven't explicitly specified the buffer sizes 
so it may well be important.

Comment 6

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

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

Changing review request to darin, although I could sr if he's happy with it.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #5)
> But, as Brendan points out, you haven't explicitly specified the buffer sizes 
> so it may well be important.

well, it has the size as specified by those zeroes there, no?

Comment 8

[Darin Fisher]

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

why not support a radix parameter for PRInt64?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

I didn't feel it was worth the code, considering that the expected use case is
for displaying filesizes and the like in the UI or .rdf storage. I can add it if
you want.

Comment 10

[Darin Fisher]

Well, since it's not much code to support the radix parameter, maybe it is worth
it.  That way the API is consistent, and maybe there will be some use for it in
the future :-/

Or, we could just say... don't add code unless you need it for something. 
Balance that with the fact that folks will probably tend to just use PR_snprintf
in their code instead of using AppendInt when they find that AppendInt doesn't
suite their needs.

My vote is to support the radix parameter.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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

ok

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

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

ok, this patch now supports a radix parameter, for consistency.

it also does away with initializing buf to zeroes, and just declares buf[20].

Comment 14

[Darin Fisher]

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

>+      case 16:
>+        fmt = "%x";
>+#ifdef DEBUG
>+        break;
>+      default:
>+        NS_NOTREACHED("Invalid radix!");
>+#endif

nit: is this #ifdef DEBUG really necessary?


>+    PR_snprintf(buf, sizeof(buf), fmt, aInteger);
>+
>     AppendWithConversion(buf);

nit: bag this extra newline


thanks for adding the testcase!

r=darin

Comment 15

[Darin Fisher]

Also, instead of calling AppendWithConversion it might be good to use
AppendASCIItoUTF16 directly.  That way you avoid some function call overhead.

Comment 16

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

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

sr=dbaron with darin's comments, although also I'm not sure what the point of
0-initializing buf is.	It seems better to either initialize fmt to the empty
string or change the |case 10:| to |default:| (and move it to the end) and have
an assertion in it that |aRadix == 10|.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #16)
> (From update of attachment 145776 [details] [diff] [review])
> sr=dbaron with darin's comments, although also I'm not sure what the point of
> 0-initializing buf is.

Hm? The latest patch no longer inits buf to zero, does it?

>	It seems better to either initialize fmt to the empty
> string or change the |case 10:| to |default:| (and move it to the end) and have
> an assertion in it that |aRadix == 10|.

Good idea, I'll do that. Although... assuming switch is implemented using a jump
table, wouldn't it be more efficient to change |case 16:| to |default:| and
assert that radix==16?

Comment 18

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

(In reply to comment #17)
> Hm? The latest patch no longer inits buf to zero, does it?

Right.  So never mind.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

hm. this actually breaks the build... AppendInt(some_pruint32_var) is now ambiguous.

Not sure what a good solution would be... should I add an AppendInt(PRUint32
var, PRInt32 radix) { AppendInt(PRInt32(var), radix); } ? It would not change
the behaviour, although it would produce incorrect results for numbers above
PR_INT32_MAX...

Comment 20

[tenthumbs]

> +void
> +nsCString::AppendInt( PRInt64 aInteger, PRInt32 aRadix )
> +  {
> +    char buf[22];

AppendInt (-1, 8) needs 23 bytes.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #20)
> AppendInt (-1, 8) needs 23 bytes.

Ew, I only thought about 10 :(

I'll attach a new patch

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: final patch

all comments addressed. I also added a testcase for AppendInt(LL_MinInt(), 8)
(which needs the same amount of digits as -1, in octal)

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 145880 [details] [diff] [review]
final patch

this would be nice to have for 1.7. very low risk - makes small changes to an
existing function, and adds two new functions

(I'd like to get this in for the bug that depends on this one)

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 145880 [details] [diff] [review]
final patch

a=asa (on behalf of drivers) for checkin to 1.7

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in to trunk, leaving open for 1.7 branch checkin

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: bustage fix

this is the bustage fix I checked in yesterday

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed on 1.7 branch

Comment 28

[cls]

Attachment: fix mingw bustage

I'm not sure why the cross-compiled builds didn't pick this up but I'm seeing
the same NS_COM/inline bustage that we saw in bug 231995 & bug 226609 in my
native mingw builds on the 1.7 branch & the trunk.  This patch fixes the
problem.

Comment 29

[David G King]

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

Comment 30

[David G King]

The mingw patch fixes my problems from Bug #240917, thanks cls.

Comment 31

[Darin Fisher]

Comment on attachment 146399 [details] [diff] [review]
fix mingw bustage

doh!  of course.  i should have caught that in my original review :-(

Comment 32

[cls]

Comment on attachment 146399 [details] [diff] [review]
fix mingw bustage

This fix has been checked into the trunk.

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 146399 [details] [diff] [review]
fix mingw bustage

a=asa (on behalf of drivers) for checkin to 1.7

Comment 34

[cls]

The mingw fix has been checked into the 1.7 branch.

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

This patch caused build bustage for DEBUG_CAPS_HACKER.  I checked in the obvious
casting fix to MOZILLA_1_7_BRANCH, but this probably wants to have a better fix
on HEAD (making |union SecurityLevel| keep a PR type instead of a long).  Biesi,
care to have a go at that?

Also did this patch make it to any other (aviary?) branches?  I will likely want
to debug those at some point so it would be nice if you could push a fix to
whatever other branches are infected.

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: fix DEBUG_CAPS_HACKER bustage

changes long to PRInt32

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

DEBUG_CAPS_HACKER fix checked in.
Checking in nsScriptSecurityManager.h;
/cvsroot/mozilla/caps/include/nsScriptSecurityManager.h,v  <-- 
nsScriptSecurityManager.h
new revision: 1.80; previous revision: 1.79
done

Comment 38

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

This landed on the 1.7 branch well before the Aviary 1.0 branch branched off of 1.7.