Closed Bug 792581 Opened 7 years ago Closed 7 years ago

Remove LL_MUL, LL_Zero() and similar 64-bit helpers

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: Biesinger, Assigned: drexler)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(23 files, 7 obsolete files)

5.92 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.18 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.88 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.27 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
9.67 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.87 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
944 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
793 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
7.61 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
11.12 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
22.44 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
15.53 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.16 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.67 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
12.51 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
11.97 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.48 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
5.16 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
19.41 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
17.59 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.42 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
19.80 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
21.07 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
We recently removed PRInt64/PRUint64 in favor of int64_t/uint64_t. This means we definitely don't need to use LL_MUL, LL_Zero(), LL_ZERO et al anymore.

This bug is for replacing them with standard operators and with 0.

http://mxr.mozilla.org/mozilla-central/search?string=LL_MUL
http://mxr.mozilla.org/mozilla-central/search?string=LL_Zero&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

I am primilarly converned about LL_Zero() because that's an actual function call unlike the rest, so this actually has some performance implications.
See http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prlong.h#58 and below for a list of these horrible macros.  For each one, we should replace all of their occurrences with regular math operators, everywhere outside of NSPR and NSS.

It's probably best to do each macro in its own patch!

CCing Isaac to see if he's interested in this when he has some time.
Whiteboard: [mentor=ehsan][lang=c++]
Assignee: nobody → andrew.quartey
Attached patch part 1: replace LL_IS_ZERO (obsolete) — Splinter Review
Attachment #665717 - Flags: review?(ehsan)
Attached patch part 2: Replace LL_EQ (obsolete) — Splinter Review
Attachment #665718 - Flags: review?(ehsan)
Attached patch part 3: replace LL_NE macro (obsolete) — Splinter Review
Attachment #665719 - Flags: review?(ehsan)
Attached patch part 4: replace LL_GE_ZERO macro (obsolete) — Splinter Review
Attachment #665720 - Flags: review?(ehsan)
Attached patch part 5: replace LL_CMP macro (obsolete) — Splinter Review
Attachment #665721 - Flags: review?(ehsan)
Attached patch part 6: replace LL_UCMP macro (obsolete) — Splinter Review
Attachment #665722 - Flags: review?(ehsan)
Part 1-6 replaces the relational operator macros and a patch for each part is _dependent_ on the one preceding it. Sent to TRY: https://tbpl.mozilla.org/?tree=Try&rev=1fdd7f5b34a3.
Try run for 1fdd7f5b34a3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1fdd7f5b34a3
Results (out of 217 total builds):
    success: 195
    warnings: 20
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/andrew.quartey@gmail.com-1fdd7f5b34a3
Please exclude nsprpub from these changes; it's separately managed and has different rules.
Ah, I see you even removed the macros. NSPR is strictly backwards compatible; please keep the macros.
Comment on attachment 665717 [details] [diff] [review]
part 1: replace LL_IS_ZERO

Yes, please don't touch any files in nsprpub/ and security/nss, as we don't own that code.

Sorry, I should have been clearer about this.
Attachment #665717 - Flags: review?(ehsan)
Attachment #665718 - Flags: review?(ehsan)
Attachment #665719 - Flags: review?(ehsan)
Attachment #665720 - Flags: review?(ehsan)
Attachment #665721 - Flags: review?(ehsan)
Attachment #665722 - Flags: review?(ehsan)
Attachment #665717 - Attachment is obsolete: true
Attachment #665718 - Attachment is obsolete: true
Attachment #665719 - Attachment is obsolete: true
Attachment #665720 - Attachment is obsolete: true
Attachment #665721 - Attachment is obsolete: true
Attachment #665722 - Attachment is obsolete: true
Attachment #667306 - Flags: review?(ehsan)
Attachment #667307 - Flags: review?(ehsan)
Attachment #667308 - Flags: review?(ehsan)
Attachment #667309 - Flags: review?(ehsan)
Attachment #667310 - Flags: review?(ehsan)
Attachment #667311 - Flags: review?(ehsan)
Attachment #667306 - Flags: review?(ehsan) → review+
Attachment #667307 - Flags: review?(ehsan) → review+
Attachment #667308 - Flags: review?(ehsan) → review+
Attachment #667309 - Flags: review?(ehsan) → review+
Attachment #667310 - Flags: review?(ehsan) → review+
Attachment #667311 - Flags: review?(ehsan) → review+
Comment on attachment 667310 [details] [diff] [review]
part 5: replace LL_CMP macro

>-  if (LL_CMP(bytes, >, INT64_MAX))
>+  if (bytes > INT64_MAX)
Oh, an actual bug has been fixed beyond a cleanup! It was expanded to
((PRInt64)(bytes) > (PRInt64)(INT64_MAX))
which would be never true.
Try run for b2f5987a94c0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b2f5987a94c0
Results (out of 230 total builds):
    success: 216
    warnings: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/andrew.quartey@gmail.com-b2f5987a94c0
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++] [leave open]
Duplicate of this bug: 786542
Remember to also replace the macros and functions above ehsan's comment 1 link (LL_MAXINT, LL_Zero(), and the like). Replace with 0 or INT64_MAX, etc.
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #23)
> Remember to also replace the macros and functions above ehsan's comment 1
> link (LL_MAXINT, LL_Zero(), and the like). Replace with 0 or INT64_MAX, etc.

I plan to take care of those after submitting patches for the shift and conversion macros.
Attachment #668046 - Flags: review?(ehsan)
Attachment #668047 - Flags: review?(ehsan)
Attachment #668048 - Flags: review?(ehsan)
Attachment #668050 - Flags: review?(ehsan)
Attachment #668051 - Flags: review?(ehsan)
Attachment #668053 - Flags: review?(ehsan)
Attachment #668055 - Flags: review?(ehsan)
Comment on attachment 668048 [details] [diff] [review]
part 9: replace LL_ADD macro

use ++ instead of += 1?
Attachment #668046 - Flags: review?(ehsan) → review+
Attachment #668047 - Flags: review?(ehsan) → review+
Comment on attachment 668048 [details] [diff] [review]
part 9: replace LL_ADD macro

Review of attachment 668048 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUserFontSet.cpp
@@ +626,2 @@
>      if (sFontSetGeneration == 0)
> +       sFontSetGeneration += 1;

Nit: please use ++.

::: security/manager/ssl/src/nsCRLManager.cpp
@@ +418,5 @@
>      LL_SUB(diff, now, lastUpdate);             //diff is the no of micro sec between now and last update
>      LL_DIV(cycleCnt, diff, microsecInDayCnt);   //temp is the number of full cycles from lst update
>      LL_MOD(temp, diff, microsecInDayCnt);
>      if(temp != 0) {
> +      cycleCnt += 1;            //no of complete cycles till next autoupdate instant

Here too.
Attachment #668048 - Flags: review?(ehsan) → review+
Attachment #668050 - Flags: review?(ehsan) → review+
Attachment #668051 - Flags: review?(ehsan) → review+
Comment on attachment 668053 [details] [diff] [review]
part 12: replace LL_DIV macro

Review of attachment 668053 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/CvtURL.cpp
@@ +78,5 @@
>    PRTime end = PR_Now();
>    PRTime conversion, ustoms;
>    LL_I2L(ustoms, 1000);
>    conversion = end - start;
> +  conversion /= ustoms;

Nit: conversion = (end - start) / ustoms;
Attachment #668053 - Flags: review?(ehsan) → review+
Attachment #668055 - Flags: review?(ehsan) → review+
Attachment #669305 - Flags: review?(ehsan)
Attachment #669305 - Attachment description: part 14: Replace LL_L2I macro → part 15: Replace LL_L2I macro
Attachment #669306 - Flags: review?(ehsan)
Attachment #669307 - Flags: review?(ehsan)
Attachment #669308 - Flags: review?(ehsan)
Attachment #669309 - Flags: review?(ehsan)
Attachment #669303 - Flags: review?(ehsan) → review+
Comment on attachment 669305 [details] [diff] [review]
part 15: Replace LL_L2I macro

Review of attachment 669305 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsContentSink.cpp
@@ +1236,1 @@
>      delay /= PR_USEC_PER_MSEC;

Nit: combine these two lines please.

::: layout/xul/base/src/nsListBoxBodyFrame.cpp
@@ +908,5 @@
>    PRTime end = PR_Now();
>  
>    PRTime difTime = end - start;
>  
> +  int32_t newTime = int32_t(difTime);

Nit: you should be able to get rid of difTime here.

::: xpcom/reflect/xptcall/tests/TestXPTCInvoke.cpp
@@ +397,1 @@
>           printf("\t3 + 5 + 7 + 11 + 13 + 17 + 19 + 23 + 29 + 31 = %d\n", (int)tmp32);

In these call sites, it doesn't make a lot of sense to first cast into an int32_t and then into an int.  You should just be able to cast directly to int and remove the casts in the printf lines.
Attachment #669305 - Flags: review?(ehsan) → review+
Comment on attachment 669306 [details] [diff] [review]
part 16: Replace LL_L2UI macro

Review of attachment 669306 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/pref/autoconfig/src/nsAutoConfig.cpp
@@ +421,2 @@
>      file->GetFileSize(&fileSize);
> +    uint32_t fs = uint32_t(fileSize); // Converting 64 bit structure to unsigned int

Nit: no need for the cast.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +770,5 @@
>  
>              // Make sure the file map is closed, no matter how we return.
>              FileMapAutoCloser mapCloser(map);
>  
> +            uint32_t fileSize32 = uint32_t(fileSize);

Nit: no need for the cast.
Attachment #669306 - Flags: review?(ehsan) → review+
Attachment #669307 - Flags: review?(ehsan) → review+
Attachment #669308 - Flags: review?(ehsan) → review+
Comment on attachment 669309 [details] [diff] [review]
part 19: Replace LL_I2L macro

Review of attachment 669309 [details] [diff] [review]:
-----------------------------------------------------------------

(Sorry for the large number of nits :)

::: content/base/src/nsContentSink.cpp
@@ +1228,2 @@
>  
> +    int64_t interval = int64_t(GetNotificationInterval());

Nit: no need for the cast.

@@ +1234,1 @@
>      delay /= PR_USEC_PER_MSEC;

This could be simplified a lot if you get rid of `diff' and also do the division when `delay' is being initialized.

::: docshell/base/nsDocShell.h
@@ +531,5 @@
>  
>      static  inline  uint32_t
>      PRTimeToSeconds(PRTime t_usec)
>      {
> +      PRTime usec_per_sec = int64_t(PR_USEC_PER_SEC);

The cast should not be needed here.

@@ +536,2 @@
>        t_usec /= usec_per_sec;
>        return  uint32_t(t_usec);

This could also be fairly easily simplified into a one-liner.

::: netwerk/base/src/nsProtocolProxyService.cpp
@@ +781,5 @@
>      // get time elapsed since session start
>      int64_t diff = now - mSessionStart;
>  
>      // convert microseconds to seconds
> +    diff /= int64_t(PR_USEC_PER_SEC);

Nit: the cast is not needed here, since `diff' is already int64_t.

::: netwerk/cache/nsCache.cpp
@@ +44,5 @@
>  
>  uint32_t
>  SecondsFromPRTime(PRTime prTime)
>  {
> +  int64_t  microSecondsPerSecond = int64_t(PR_USEC_PER_SEC);

Nit: no cast needed.

@@ +58,2 @@
>    LL_UI2L(intermediateResult, seconds);
> +  PRTime prTime = intermediateResult * int64_t(PR_USEC_PER_SEC);

Nit: the cast is not needed here either.

::: netwerk/dns/nsHostResolver.cpp
@@ +93,1 @@
>      return uint32_t(now / factor);

Nit: this can also be simplified into a one-liner.

::: netwerk/protocol/about/nsAboutCache.cpp
@@ +21,5 @@
>  static PRTime SecondsToPRTime(uint32_t t_sec)
>  {
>      PRTime t_usec, usec_per_sec;
> +    t_usec = int64_t(t_sec);
> +    usec_per_sec = int64_t(PR_USEC_PER_SEC);

Nit: these casts are not needed.

::: netwerk/protocol/about/nsAboutCacheEntry.cpp
@@ +178,5 @@
>  static PRTime SecondsToPRTime(uint32_t t_sec)
>  {
>      PRTime t_usec, usec_per_sec;
> +    t_usec = int64_t(t_sec);
> +    usec_per_sec = int64_t(PR_USEC_PER_SEC);

Ditto.

::: tools/trace-malloc/spacetrace.c
@@ +3870,5 @@
>                           ** Need to do this math in 64 bits.
>                           */
> +                        ydata64 = int64_t(YData[traverse]);
> +                        spacey64 = int64_t(STGD_SPACE_Y);
> +                        mem64 = int64_t(maxMemory - minMemory);

Hmm, this is a .c file, so you should not use constructor style casts...

@@ +4086,5 @@
>                           ** Need to do this math in 64 bits.
>                           */
> +                        ydata64 = int64_t(YData[traverse]);
> +                        spacey64 = int64_t(STGD_SPACE_Y);
> +                        mem64 = int64_t(maxMemory - minMemory);

Ditto.

@@ +4304,5 @@
>                           ** Need to do this math in 64 bits.
>                           */
> +                        ydata64 = int64_t(YData[traverse]);
> +                        spacey64 = int64_t(STGD_SPACE_Y);
> +                        mem64 = int64_t(maxMemory - minMemory);

And here.

@@ +4535,5 @@
>  
>                          /*
>                           ** Need to do this math in 64 bits.
>                           */
> +                        spacey64 = int64_t(STGD_SPACE_Y);

Here too.

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +56,5 @@
>  
>  static inline uint32_t
>  PRTimeToSeconds(PRTime t_usec)
>  {
> +    PRTime usec_per_sec = int64_t(PR_USEC_PER_SEC);

Nit: cast not needed, and the same one-liner suggestion applies here.

::: xpcom/ds/nsVariant.cpp
@@ +646,5 @@
>          return rv;
>      switch(tempData.mType)
>      {
>      case nsIDataType::VTYPE_INT32:
> +        *_retval = int64_t(tempData.u.mInt32Value);

Nit: the cast is not needed.

::: xpcom/glue/nsTextFormatter.cpp
@@ +286,5 @@
>      ** Converting decimal is a little tricky. In the unsigned case we
>      ** need to stop when we hit 10 digits. In the signed case, we can
>      ** stop when the number is zero.
>      */
> +    rad = int64_t(radix);

Cast not needed here as well.

::: xpcom/io/nsLocalFileOS2.cpp
@@ +1713,5 @@
>      if (NS_FAILED(rv))
>          return rv;
>  
>      // microseconds -> milliseconds
> +    *aLastModifiedTime = mFileInfo64.modifyTime / int64_t(PR_USEC_PER_MSEC);

Ditto.

::: xpcom/io/nsLocalFileWin.cpp
@@ +2265,5 @@
>      if (NS_FAILED(rv))
>          return rv;
>  
>      // microseconds -> milliseconds
> +    *aLastModifiedTime = mFileInfo64.modifyTime / int64_t(PR_USEC_PER_MSEC);

Ditto.

@@ +2287,5 @@
>      if (NS_FAILED(rv)) 
>          return rv;
>  
>      // microseconds -> milliseconds
> +    *aLastModifiedTime = info.modifyTime / int64_t(PR_USEC_PER_MSEC);

Here too.

::: xpcom/reflect/xptcall/tests/TestXPTCInvoke.cpp
@@ +324,5 @@
>      else
>          printf("\tFAILED");
>      int64_t one, two;
> +    one = 1;
> +    two = 2;

Nit: please initialize these two in the line above.
Attachment #669309 - Flags: review?(ehsan) → review+
Attachment #673593 - Flags: review?(ehsan)
Comment on attachment 673593 [details] [diff] [review]
part 20: Replace LL_UI2L macro

Review of attachment 673593 [details] [diff] [review]:
-----------------------------------------------------------------

(Note that the static_casts comments are not really nits, cause without those this patch introduces bugs.)

::: netwerk/streamconv/converters/ParseFTPList.cpp
@@ +486,5 @@
>                 * So its rounded up to the next block, so what, its better
>                 * than not showing the size at all.
>                */
> +              uint64_t fsz = strtoul(tokens[1], (char **)0, 10);
> +              fsz *= 512;

Nit: please do the multiplication in one go with static_casting stuff to 64-bit ints.

::: tools/trace-malloc/spacetrace.c
@@ +112,5 @@
>  ticks2xsec(tmreader * aReader, uint32_t aTicks, uint32_t aResolution)
>  {
> +    uint64_t bigone = aResolution;
> +    bigone *= aTicks;
> +    bigone /= aReader->ticksPerSec;

Nit: same here, you can calculate `bigone' in one go with static_casting stuff to 64-bit ints.

@@ +951,5 @@
>  
>                  /*
>                   ** Check weight restrictions.
>                   */
> +                weight64 = bytesize * lifetime;

You need to static_cast these variables to be 64-bit variables so that the multiplication doesn't overflow 32 bits.

@@ +2710,3 @@
>                      char buffer[32];
>  
> +                    weight64 = size * lifespan;

static_cast here too.

@@ +2830,3 @@
>                      char buffer[32];
>  
> +                    weight64 = size * lifespan;

Ditto.

@@ +3084,4 @@
>          uint32_t cacheval = 0;
>          int displayRes = 0;
>  
> +        weight64 = bytesize * timeval;

Same here.

::: tools/trace-malloc/tmfrags.c
@@ +417,5 @@
>  */
>  {
> +    uint64_t bigone = aResolution;
> +    bigone *= aTicks;
> +    bigone /= aReader->ticksPerSec;

Nit: please do this in one go with static_casts.

::: tools/trace-malloc/tmstats.c
@@ +415,5 @@
>  {
>      uint64_t squared;
>      uint64_t bigValue;
> +
> +    bigValue = inValue;

Nit: please initialize and declare in one go.

@@ +532,4 @@
>  
> +    bigone = aResolution;
> +    bigone *= aTicks;
> +    bigone /= aReader->ticksPerSec;

Same nit as before.  :-)
Attachment #673593 - Flags: review?(ehsan) → review+
Comment on attachment 673595 [details] [diff] [review]
part 21: Replace LL_F2L and LL_D2L macros

Review of attachment 673595 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/src/mozStoragePrivateHelpers.cpp
@@ +141,5 @@
>        return nullptr;
>  
>      double msecd = ::js_DateGetMsecSinceEpoch(obj);
>      msecd *= 1000.0;
> +    int64_t msec = int64_t(msecd);

No need for the cast here.

::: xpcom/ds/nsVariant.cpp
@@ +653,5 @@
>          *_retval = tempData.u.mUint32Value;
>          return rv;
>      case nsIDataType::VTYPE_DOUBLE:
>          // XXX should check for data loss here!
> +        *_retval = int64_t(tempData.u.mDoubleValue);

No need for the cast here too.
Attachment #673595 - Flags: review?(ehsan) → review+
Attachment #673597 - Flags: review?(ehsan) → review+
Attached patch part 23: Replace LL_INIT macro (obsolete) — Splinter Review
Attachment #677486 - Flags: review?(ehsan)
Comment on attachment 677486 [details] [diff] [review]
part 23: Replace LL_INIT macro

Review of attachment 677486 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.

::: toolkit/components/places/nsNavHistory.cpp
@@ +100,5 @@
>  // for repeating stuff.  These are milliseconds between "now" cache refreshes.
>  #define RENEW_CACHED_NOW_TIMEOUT ((int32_t)3 * PR_MSEC_PER_SEC)
>  
>  // USECS_PER_DAY == PR_USEC_PER_SEC * 60 * 60 * 24;
> +static const int64_t USECS_PER_DAY = (20 << 32) + 500654080;

You need to say int64_t(20 << 32) here, or 20ll << 32, or something which guarantees that the result is a 64-bit int.

::: widget/xpwidgets/nsTransferable.cpp
@@ +175,5 @@
>    bool exists;
>    if ( cacheFile && NS_SUCCEEDED(cacheFile->Exists(&exists)) && exists ) {
>      // get the size of the file
>      int64_t fileSize;
> +    int64_t max32(0xFFFFFFFF);

Nit: please use =.
Attachment #677486 - Flags: review?(ehsan) → review+
(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> Comment on attachment 677486 [details] [diff] [review]
> part 23: Replace LL_INIT macro
> 
> Review of attachment 677486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the below addressed.
> 
> ::: toolkit/components/places/nsNavHistory.cpp
> @@ +100,5 @@
> >  // for repeating stuff.  These are milliseconds between "now" cache refreshes.
> >  #define RENEW_CACHED_NOW_TIMEOUT ((int32_t)3 * PR_MSEC_PER_SEC)
> >  
> >  // USECS_PER_DAY == PR_USEC_PER_SEC * 60 * 60 * 24;
> > +static const int64_t USECS_PER_DAY = (20 << 32) + 500654080;
> 
> You need to say int64_t(20 << 32) here, or 20ll << 32, or something which
> guarantees that the result is a 64-bit int.

Nit: int64_t(20) << 32, not int64_t(20 << 32).
Fixed nits for last reviewed patch and folded patch for LL_UDIVMOD macro into it since it only touches one file-nsTextFormatter.cpp.
Attachment #677486 - Attachment is obsolete: true
Attachment #677518 - Flags: review?(ehsan)
This should be the last patch.
Whiteboard: [mentor=ehsan][lang=c++] [leave open] → [mentor=ehsan][lang=c++]
Comment on attachment 677518 [details] [diff] [review]
part 23: Replace LL_INIT and LL_UDIVMOD macros

 // USECS_PER_DAY == PR_USEC_PER_SEC * 60 * 60 * 24;
-static const int64_t USECS_PER_DAY = LL_INIT(20, 500654080);
+static const int64_t USECS_PER_DAY = (20LL << 32) + 500654080;

This is silly. Why not:

static const int64_t USECS_PER_DAY = PR_USEC_PER_SEC * 60 * 60 * 24;

(and remove the comment)

Also, are you sure the LL suffix works on windows/msvc?
(In reply to comment #58)
> (In reply to Ehsan Akhgari [:ehsan] from comment #57)
> > Comment on attachment 677486 [details] [diff] [review]
> > part 23: Replace LL_INIT macro
> > 
> > Review of attachment 677486 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the below addressed.
> > 
> > ::: toolkit/components/places/nsNavHistory.cpp
> > @@ +100,5 @@
> > >  // for repeating stuff.  These are milliseconds between "now" cache refreshes.
> > >  #define RENEW_CACHED_NOW_TIMEOUT ((int32_t)3 * PR_MSEC_PER_SEC)
> > >  
> > >  // USECS_PER_DAY == PR_USEC_PER_SEC * 60 * 60 * 24;
> > > +static const int64_t USECS_PER_DAY = (20 << 32) + 500654080;
> > 
> > You need to say int64_t(20 << 32) here, or 20ll << 32, or something which
> > guarantees that the result is a 64-bit int.
> 
> Nit: int64_t(20) << 32, not int64_t(20 << 32).

Right!
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #61)
> Also, are you sure the LL suffix works on windows/msvc?

I believe so.
Comment on attachment 677518 [details] [diff] [review]
part 23: Replace LL_INIT and LL_UDIVMOD macros

Review of attachment 677518 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with biesi's comment addressed.

Also, thanks a lot for your work here, Andrew!
Attachment #677518 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/966f596586fd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(In reply to Ehsan Akhgari [:ehsan] from comment #63)
> (In reply to Christian :Biesinger (don't email me, ping me on IRC) from
> comment #61)
> > Also, are you sure the LL suffix works on windows/msvc?

This is what (U)INT64_C(x) is for.

(In reply to Ehsan Akhgari [:ehsan] from comment #53)
> ::: storage/src/mozStoragePrivateHelpers.cpp
> @@ +141,5 @@
> >        return nullptr;
> >  
> >      double msecd = ::js_DateGetMsecSinceEpoch(obj);
> >      msecd *= 1000.0;
> > +    int64_t msec = int64_t(msecd);
> 
> No need for the cast here.

Pretty sure at least Windows will complain about implicit conversion from double to int64_t.
(In reply to comment #67)
> (In reply to Ehsan Akhgari [:ehsan] from comment #53)
> > ::: storage/src/mozStoragePrivateHelpers.cpp
> > @@ +141,5 @@
> > >        return nullptr;
> > >  
> > >      double msecd = ::js_DateGetMsecSinceEpoch(obj);
> > >      msecd *= 1000.0;
> > > +    int64_t msec = int64_t(msecd);
> > 
> > No need for the cast here.
> 
> Pretty sure at least Windows will complain about implicit conversion from
> double to int64_t.

Ah, right...  Can you please file a follow-up about this?  Thanks!
You need to log in before you can comment on or make changes to this bug.