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

RESOLVED FIXED in mozilla1.7final

Status

()

Core
String
--
enhancement
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

(Blocks: 1 bug, {fixed1.7})

Trunk
mozilla1.7final
fixed1.7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

I have a patch for this; also cleans up AppendInt (to use PR_snprintf)
Attachment #145756 - Flags: review?(dbaron)

Comment 2

14 years ago
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.
I'm not interested in PR_snprintf's return value here.
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

14 years ago
> 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 on attachment 145756 [details] [diff] [review]
patch

Changing review request to darin, although I could sr if he's happy with it.
Attachment #145756 - Flags: review?(dbaron) → review?(darin)
(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

14 years ago
Comment on attachment 145756 [details] [diff] [review]
patch

why not support a radix parameter for PRInt64?
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

14 years ago
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 on attachment 145756 [details] [diff] [review]
patch

ok
Attachment #145756 - Attachment is obsolete: true
Attachment #145756 - Flags: review?(darin)
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].
Attachment #145776 - Flags: review?(darin)

Comment 14

14 years ago
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
Attachment #145776 - Flags: review?(darin) → review+

Comment 15

14 years ago
Also, instead of calling AppendWithConversion it might be good to use
AppendASCIItoUTF16 directly.  That way you avoid some function call overhead.
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|.
Attachment #145776 - Flags: superreview+
(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?

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

Right.  So never mind.
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...
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha

Comment 20

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

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

Ew, I only thought about 10 :(

I'll attach a new patch
Created attachment 145880 [details] [diff] [review]
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)
Attachment #145776 - Attachment is obsolete: true
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)
Attachment #145880 - Flags: approval1.7?
Target Milestone: mozilla1.8alpha → mozilla1.7final

Comment 24

14 years ago
Comment on attachment 145880 [details] [diff] [review]
final patch

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #145880 - Flags: approval1.7? → approval1.7+
checked in to trunk, leaving open for 1.7 branch checkin
Created attachment 146110 [details] [diff] [review]
bustage fix

this is the bustage fix I checked in yesterday
fixed on 1.7 branch
Keywords: fixed1.7
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 28

14 years ago
Created attachment 146399 [details] [diff] [review]
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.

Updated

14 years ago
Attachment #146399 - Flags: review?(darin)

Comment 29

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

Comment 30

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

Comment 31

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

doh!  of course.  i should have caught that in my original review :-(
Attachment #146399 - Flags: review?(darin) → review+

Comment 32

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

This fix has been checked into the trunk.
Attachment #146399 - Flags: approval1.7?

Comment 33

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

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #146399 - Flags: approval1.7? → approval1.7+

Comment 34

14 years ago
The mingw fix has been checked into the 1.7 branch.
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.
Created attachment 150391 [details] [diff] [review]
fix DEBUG_CAPS_HACKER bustage

changes long to PRInt32
Attachment #150391 - Flags: review?(caillon)

Updated

14 years ago
Whiteboard: needed-aviary1.0
Attachment #150391 - Flags: review?(caillon) → review+
Attachment #150391 - Flags: superreview?(darin)

Updated

14 years ago
Attachment #150391 - Flags: superreview?(darin) → superreview+
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
This landed on the 1.7 branch well before the Aviary 1.0 branch branched off of 1.7.
Whiteboard: needed-aviary1.0
You need to log in before you can comment on or make changes to this bug.