Closed Bug 1118076 Opened 5 years ago Closed 5 years ago

Drop MOZ_THIS_IN_INITIALIZER_LIST

Categories

(Core :: MFBT, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: emk, Assigned: cpeterson)

References

Details

Attachments

(2 files)

C4355 has been disabled by default since MSVC2012.
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Depends on: 839998
1. Find and replace s/MOZ_THIS_IN_INITIALIZER_LIST()/this/
2. Reindent some some lines shortened without MOZ_THIS_IN_INITIALIZER_LIST
3. Suppress C4355 warnings in security/certverifier/moz.build because this moz.build file explicitly enabled MSVC's -Wall, which reenabled disabled-by-default C4355.
Attachment #8546459 - Flags: review?(jwalden+bmo)
Comment on attachment 8546459 [details] [diff] [review]
remove-MOZ_THIS_IN_INITIALIZER_LIST.patch

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

::: dom/bindings/TypedArray.h
@@ +300,5 @@
>  public:
>    explicit RootedTypedArray(JSContext* cx MOZ_GUARD_OBJECT_NOTIFIER_PARAM) :
>      ArrayType(),
>      TypedArrayRooter<ArrayType>(cx,
> +                                this MOZ_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT)

Use this formatting instead:

  TypedArrayRooter<ArrayType>(cx, this
                              MOZ_GUARD_OBJEcT_NOTIFIER_PARAM_TO_PARENT)

What we want is all the arguments strung out, with wrapping whenever needed -- and we want the trailing macro on its own line entirely, separate from everything else, hopefully not muddying up the scannability of the actual arguments too much.

@@ +307,5 @@
>  
>    RootedTypedArray(JSContext* cx, JSObject* obj MOZ_GUARD_OBJECT_NOTIFIER_PARAM) :
>      ArrayType(obj),
>      TypedArrayRooter<ArrayType>(cx,
> +                                this MOZ_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT)

Same thing: cx and this on the same line, the trailing macro on a new line.

::: dom/media/webaudio/OscillatorNode.cpp
@@ -21,5 @@
>  NS_INTERFACE_MAP_END_INHERITING(AudioNode)
>  
>  NS_IMPL_ADDREF_INHERITED(OscillatorNode, AudioNode)
>  NS_IMPL_RELEASE_INHERITED(OscillatorNode, AudioNode)
> -

Leave this blank line here, please.
Attachment #8546459 - Flags: review?(jwalden+bmo) → review+
Thanks, Waldo.

Landed with fixed formatting:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cadb53efd449
https://hg.mozilla.org/mozilla-central/rev/cadb53efd449
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
FWIW, there is one MOZ_THIS_IN_INITIALIZER_LIST reference left in the tree:
 image/src/SurfaceCache.cpp:#include "mozilla/Attributes.h"  // for MOZ_THIS_IN_INITIALIZER_LIST
Remove unnecessary #include "mozilla/Attributes.h" now that we've removed MOZ_THIS_IN_INITIALIZER_LIST.
Attachment #8562437 - Flags: review?(seth)
Comment on attachment 8562437 [details] [diff] [review]
fix-SurfaceCache.patch

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

::: image/src/SurfaceCache.cpp
@@ -10,5 @@
>  #include "SurfaceCache.h"
>  
>  #include <algorithm>
>  #include "mozilla/Assertions.h"
> -#include "mozilla/Attributes.h"  // for MOZ_THIS_IN_INITIALIZER_LIST

Note the comment is actually misleading. MOZ_THIS_IN_INITIALIZER_LIST is not the only thing Attributes.h is included for. For instance, this file contains MOZ_FINAL, which comes from that header as well. Sure, it might build without the include at all because in all likeliness, it's included from somewhere else. It should still be included here, though.
(In reply to Mike Hommey [:glandium] from comment #7)
> Note the comment is actually misleading.

Yep. I suspect that comment was accurate when it was added, but rapidly got out of date because there's nothing that enforces that it be correct. We might be better off removing such comments if we're not going to regularly use IWYU or something similar to check them.
Comment on attachment 8562437 [details] [diff] [review]
fix-SurfaceCache.patch

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

r+ if you remove the comment, but not the #include.
Attachment #8562437 - Flags: review?(seth) → review+
You need to log in before you can comment on or make changes to this bug.