1118076 - Drop MOZ_THIS_IN_INITIALIZER_LIST

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Masatoshi Kimura [:emk]]

C4355 has been disabled by default since MSVC2012.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: remove-MOZ_THIS_IN_INITIALIZER_LIST.patch

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.

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Chris Peterson [:cpeterson]]

Thanks, Waldo.

Landed with fixed formatting:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cadb53efd449

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cadb53efd449

Comment 5

[Mike Hommey [:glandium]]

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

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: fix-SurfaceCache.patch

Remove unnecessary #include "mozilla/Attributes.h" now that we've removed MOZ_THIS_IN_INITIALIZER_LIST.

Comment 7

[Mike Hommey [:glandium]]

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.

Comment 8

[Seth Fowler [:seth] [:s2h]]

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

[Seth Fowler [:seth] [:s2h]]

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.

Comment 10

[Chris Peterson [:cpeterson]]

Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/922c04acbb95

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/922c04acbb95