Last Comment Bug 792180 - Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY
: Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: maligree
:
Mentors:
Depends on: 792689 806762
Blocks: 730100
  Show dependency treegraph
 
Reported: 2012-09-18 12:47 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-10-30 08:27 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY across tree (264.44 KB, patch)
2012-10-25 14:02 PDT, maligree
Ms2ger: feedback+
Details | Diff | Splinter Review
Fixed ordering, indentation, and removed trailing whitespaces (265.11 KB, patch)
2012-10-26 06:35 PDT, maligree
ehsan: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-09-18 12:47:25 PDT
Currently, nscore.h [1] has NS_LIKELY and NS_UNLIKELY. MOZ_LIKELY and MOZ_UNLIKELY in mozilla/Likely.h [2] are better alternatives. The purpose of this bug is to

* Replace all occurrences of NS_LIKELY and NS_UNLIKELY [3,4] with the MOZ_* variants;
* Add #include "mozilla/Likely.h" to all touched files;
* Remove the definitions of NS_LIKELY and NS_UNLIKELY from nscore.h

Since there are a few hundred occurrences of these macros, it would probably be easiest to write a script to automate the first part. You could use [5] for inspiration.

Feel free to ask me if you have any questions.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h#366
[2] http://mxr.mozilla.org/mozilla-central/source/mfbt/Likely.h
[3] http://mxr.mozilla.org/mozilla-central/ident?i=NS_LIKELY
[4] http://mxr.mozilla.org/mozilla-central/ident?i=NS_UNLIKELY
[5] https://bug579517.bugzilla.mozilla.org/attachment.cgi?id=653946
Comment 1 Mats Palmgren (:mats) 2012-09-19 21:40:18 PDT
> MOZ_LIKELY and MOZ_UNLIKELY in mozilla/Likely.h [2] are better alternatives.

No, they are worse alternatives.  They fail to compile:
  if (MOZ_LIKELY(f))  // f is a nsIFrame*

(see bug 340244 for details)

I do agree with the naming change though, but please change the
MOZ_ macros to match the NS_ ones first.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-09-20 05:24:31 PDT
(In reply to Mats Palmgren [:mats] from comment #1)
> > MOZ_LIKELY and MOZ_UNLIKELY in mozilla/Likely.h [2] are better alternatives.
> 
> No, they are worse alternatives.  They fail to compile:
>   if (MOZ_LIKELY(f))  // f is a nsIFrame*
> 
> (see bug 340244 for details)
> 
> I do agree with the naming change though, but please change the
> MOZ_ macros to match the NS_ ones first.

This has been fixed.
Comment 3 tienthanh8490 2012-09-28 03:25:44 PDT
Hi Ms2ger, i would like to help on this bug. Could you please assist me with this ? Thank you.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-09-28 06:08:05 PDT
(In reply to tienthanh8490 from comment #3)
> Hi Ms2ger, i would like to help on this bug. Could you please assist me with
> this ? Thank you.

Certainly. If you aren't yet, it's probably easiest to get on IRC (<https://wiki.mozilla.org/IRC>), and join the #introduction channel.
Comment 5 chronon 2012-10-17 12:43:58 PDT
tienthanh8490, are you still working on this bug? If not, I'd be happy to take it over
Comment 6 maligree 2012-10-25 14:02:21 PDT
Created attachment 675285 [details] [diff] [review]
Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY across tree

It was a slow day today, so I figured..

And it even builds.
Comment 7 tienthanh8490 2012-10-25 15:34:30 PDT
(In reply to chronon from comment #5)
> tienthanh8490, are you still working on this bug? If not, I'd be happy to
> take it over

Yes i dropped out, please take it over
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-10-26 05:29:07 PDT
Comment on attachment 675285 [details] [diff] [review]
Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY across tree

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

Thanks a lot! I made some small comments below; please fix those and ask :ehsan to review.

Henri, could you check if the changes here touch generated code?

::: accessible/src/atk/nsMaiInterfaceAction.cpp
@@ +8,5 @@
>  
>  #include "Accessible-inl.h"
>  #include "nsMai.h"
>  #include "Role.h"
> +#include "mozilla/Likely.h" 

Remove the extra space at the end of this line, here and elsewhere in the patch.

::: content/base/src/nsINode.cpp
@@ +14,5 @@
>  #include "mozAutoDocUpdate.h"
>  #include "mozilla/CORSMode.h"
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Put this one between CORSMode and Telemetry, to preserve the alphabetical ordering.

::: gfx/thebes/gfxFont.cpp
@@ +33,5 @@
>  #include "mozilla/FloatingPoint.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/Services.h"
>  #include "mozilla/Telemetry.h"
> +#include "mozilla/Likely.h"

Ordering again.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +17,5 @@
>  #include "mozilla/Preferences.h"
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/Likely.h"

Sort those too

::: js/xpconnect/src/nsXPConnect.cpp
@@ +8,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Base64.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Again.

::: layout/base/nsPresShell.cpp
@@ +20,5 @@
>  
>  #include "mozilla/dom/PBrowserChild.h"
>  #include "mozilla/dom/TabChild.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Here

::: layout/generic/nsTextFrameThebes.cpp
@@ +73,5 @@
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/Util.h" // for DebugOnly
>  #include "mozilla/LookAndFeel.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/Likely.h"

Sort those too, please.

::: layout/style/nsRuleNode.cpp
@@ +43,5 @@
>  #include "mozilla/Assertions.h"
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/LookAndFeel.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Sort

::: layout/xul/base/src/nsMenuFrame.cpp
@@ +40,5 @@
>  #include "mozilla/Services.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/LookAndFeel.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/Likely.h"

And here, please.

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +571,1 @@
>                               NS_ERROR_HTMLPARSER_INTERRUPTED)) {

Fix the indentation here

::: toolkit/xre/nsAppRunner.cpp
@@ +1061,5 @@
>  #endif
>  
>  #define NS_ENSURE_TRUE_LOG(x, ret)               \
>    PR_BEGIN_MACRO                                 \
> +  if (MOZ_UNLIKELY(!(x))) {                       \

Fix the alignment of the backslash

::: xpcom/base/nsCycleCollector.cpp
@@ +140,5 @@
>  #include "mozilla/Mutex.h"
>  #include "mozilla/CondVar.h"
>  #include "mozilla/StandardInteger.h"
>  #include "mozilla/Telemetry.h"
> +#include "mozilla/Likely.h" 

Sort, please
Comment 9 maligree 2012-10-26 06:35:53 PDT
Created attachment 675534 [details] [diff] [review]
Fixed ordering, indentation, and removed trailing whitespaces
Comment 10 maligree 2012-10-26 06:38:55 PDT
Oops, shouldn't have marked the previous one as obsolete, I guess - looks like I cleared hsivonen's review? flag..? Sorry.
Comment 11 Henri Sivonen (:hsivonen) 2012-10-26 07:22:33 PDT
r+ for the generated parser files from me. There are indeed generated files being edited, but I can easily change the generator to match once this has landed.
Comment 12 :Ehsan Akhgari 2012-10-26 13:05:14 PDT
I'll try to review this over the weekend.  Sorry for the delay!
Comment 13 :Ehsan Akhgari 2012-10-29 15:00:38 PDT
Comment on attachment 675534 [details] [diff] [review]
Fixed ordering, indentation, and removed trailing whitespaces

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

Looks great, thanks!

::: layout/style/nsRuleNode.cpp
@@ +2320,5 @@
>      else                                                                      \
>        data_ = new (mPresContext) nsStyle##type_ ctorargs_;                    \
>    }                                                                           \
>                                                                                \
> +  if (MOZ_UNLIKELY(!data_))                                                    \

There's a bunch of places where this patch breaks indentation but I can fix those when landing.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-10-30 08:27:34 PDT
https://hg.mozilla.org/mozilla-central/rev/7bd96dda75f0

Note You need to log in before you can comment on or make changes to this bug.