245927 - nsUint64 would be nice

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

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

similar to nsInt64, a class for unsigned integers would be nice, for places
where the value is conceptually unsigned.

Comment 1

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

Attachment: patch

Comment 2

[Darin Fisher]

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

Isn't _t suffix reserved by POSIX?  (Though I've seen tons of code that uses
_t.)  Also, we already have a convention that is to prefix with nsT for such
template classes.  I don't see _t used anywhere else.  Maybe better not to
start a new convention?  See for example nsTHashtable.

Comment 3

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

ok, good idea, so I'll probably make that nsTInt64.

I'll attach a new patch after bsmedberg's review...

Comment 4

[Benjamin Smedberg]

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

>@@ -136,248 +139,267 @@ public:
>      * Convert a 64-bit integer to a native 64-bit integer.
>      */
>     operator PRInt64(void) const {
>         return mValue;
>     }
> 
>+    operator PRUint64(void) const {
>+        return mValue;
>+    }
>+

This doesn't look good to me, why not just do:

operator T(void) const {
  return mValue;
}

(and I agree with the nsTInt64 naming convention).

Comment 5

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

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

going to attach a new patch

Comment 6

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

(In reply to comment #4)
> This doesn't look good to me, why not just do:
> 
> operator T(void) const {
>   return mValue;
> }

some code wants to implicitly convert nsUint64 to a PRInt64, to pass to a
function that wants this (nsISeekableStream) -> build would break

Comment 7

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

Attachment: patch v2

Comment 8

[Darin Fisher]

Comment on attachment 151706 [details] [diff] [review]
patch v2

>Index: xpcom/ds/nsInt64.h

>+    nsTInt64(const PRInt64 aInt64) : mValue(aInt64) {
>+    }
>+    nsTInt64(const PRUint64 aInt64) : mValue(aInt64) {
>     }

ok, what's this about.	why not have a single constructor
that takes a |const T| or (even better perhaps) |const T&|
parameter?


>     operator PRInt64(void) const {
>         return mValue;
>     }
> 
>+    operator PRUint64(void) const {
>+        return mValue;
>+    }

Why allow automatic casting from signed to unsigned and
vice versa?  That seems like it would be a recipe for
disaster.  I don't want the compiler switching from
signed to unsigned arithmetic on me unless I purposely
make that change.  These casts should not be automatic
unless they are really necessary.  Are they?  We didn't
have them before, so I think they must not be necessary.


I'm going to minus this because I think it's wrong.  I'll
give sr+ if you can convince me otherwise or if you change
the patch ;-)

Comment 9

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

hmm. bug 247715's patch uses nsUint64 in nsInputStreamPump. now, that file
interfaces with nsISeekableStream in some places.

nsISeekableStream uses signed integers. (even for tell... I'm not sure why. Seek
of course needs a signed integer, for SEEK_CUR/SEEK_END support)

so there needs to be _some_ way to convert an nsUint64 to a signed PRInt64. and
also the other way round, since we want to be able to assign the result of
Tell() to an nsUint64... maybe that file should keep using nsInt64... hm...

Comment 10

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

hm actually.... an PRUint64 can be implicitly converted to an PRInt64... even
when that uses a non-native type, since in those cases PRInt64 is identical to
PRUint64

Comment 11

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

Attachment: patch v3

ok. use operator T, and nsTInt64(T), and cast at the callers as needed.

Comment 12

[Darin Fisher]

Comment on attachment 151758 [details] [diff] [review]
patch v3

sr=darin

Comment 13

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

Checking in nsInt64.h;
/cvsroot/mozilla/xpcom/ds/nsInt64.h,v  <--  nsInt64.h
new revision: 3.11; previous revision: 3.10
done