Closed Bug 792180 Opened 8 years ago Closed 8 years ago

Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19

People

(Reporter: Ms2ger, Assigned: maligree)

References

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(1 file, 1 obsolete file)

Fixed ordering, indentation, and removed trailing whitespaces
265.11 KB, patch
ehsan
: review+
Details | Diff | Splinter Review 
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
> 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.
Depends on: 792689
(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.
Hi Ms2ger, i would like to help on this bug. Could you please assist me with this ? Thank you.
(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.
tienthanh8490, are you still working on this bug? If not, I'd be happy to take it over
It was a slow day today, so I figured..

And it even builds.
Attachment #675285 - Flags: review?(Ms2ger)
(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 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
Attachment #675285 - Flags: review?(hsivonen)
Attachment #675285 - Flags: review?(Ms2ger)
Attachment #675285 - Flags: feedback+
Attachment #675285 - Attachment is obsolete: true
Attachment #675285 - Flags: review?(hsivonen)
Attachment #675534 - Flags: review?(ehsan)
Oops, shouldn't have marked the previous one as obsolete, I guess - looks like I cleared hsivonen's review? flag..? Sorry.
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.
Assignee: nobody → maligree
I'll try to review this over the weekend.  Sorry for the delay!
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.
Attachment #675534 - Flags: review?(ehsan) → review+
Depends on: 806762
https://hg.mozilla.org/mozilla-central/rev/7bd96dda75f0
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.