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.

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.

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

Looks good on Try. Sent to Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f729670158d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa4ea0c1d30 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee5a3b45e378 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb806f6be5d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/586070a8d3f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/066fea4aa285

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.

https://hg.mozilla.org/mozilla-central/rev/f729670158d6 https://hg.mozilla.org/mozilla-central/rev/7aa4ea0c1d30 https://hg.mozilla.org/mozilla-central/rev/ee5a3b45e378 https://hg.mozilla.org/mozilla-central/rev/bb806f6be5d0 https://hg.mozilla.org/mozilla-central/rev/586070a8d3f1 https://hg.mozilla.org/mozilla-central/rev/066fea4aa285

(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.

Comment on attachment 668048 [details] [diff] [review] part 9: replace LL_ADD macro use ++ instead of += 1?

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.

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;

Fixed nits. https://hg.mozilla.org/integration/mozilla-inbound/rev/91edf65b6279 https://hg.mozilla.org/integration/mozilla-inbound/rev/721cf3acc269 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b805f35a69 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3f56cede83 https://hg.mozilla.org/integration/mozilla-inbound/rev/05cd1044c8c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee43ffdf7dde https://hg.mozilla.org/integration/mozilla-inbound/rev/01ada5d631e7

https://hg.mozilla.org/mozilla-central/rev/91edf65b6279 https://hg.mozilla.org/mozilla-central/rev/721cf3acc269 https://hg.mozilla.org/mozilla-central/rev/b7b805f35a69 https://hg.mozilla.org/mozilla-central/rev/4c3f56cede83 https://hg.mozilla.org/mozilla-central/rev/05cd1044c8c7 https://hg.mozilla.org/mozilla-central/rev/ee43ffdf7dde https://hg.mozilla.org/mozilla-central/rev/01ada5d631e7

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.

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.

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.

Fixed nits: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7fbacd886c https://hg.mozilla.org/integration/mozilla-inbound/rev/6857ec16567d https://hg.mozilla.org/integration/mozilla-inbound/rev/14cef04d0b34 https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed2340f0df6 https://hg.mozilla.org/integration/mozilla-inbound/rev/2aef09336e78 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8338085eed

https://hg.mozilla.org/mozilla-central/rev/7a7fbacd886c https://hg.mozilla.org/mozilla-central/rev/6857ec16567d https://hg.mozilla.org/mozilla-central/rev/14cef04d0b34 https://hg.mozilla.org/mozilla-central/rev/7ed2340f0df6 https://hg.mozilla.org/mozilla-central/rev/2aef09336e78 https://hg.mozilla.org/mozilla-central/rev/4b8338085eed

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. :-)

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.

fixed nits and sent to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb440887096 https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa2637e2b6b https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a9ae0f6abf

https://hg.mozilla.org/mozilla-central/rev/5fb440887096 https://hg.mozilla.org/mozilla-central/rev/9aa2637e2b6b https://hg.mozilla.org/mozilla-central/rev/b3a9ae0f6abf

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 =.

(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.

This should be the last patch.

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!

Biesi's comments addressed. Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/966f596586fd

(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!