839998 - Consider moving ALLOW_THIS_IN_INITIALIZER_LIST to MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This macro comes from the chromium code, but has recently started being used in netwerk too. Maybe this is a better solution than the thisInInitializer() functions we've been using elsewhere.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

wdyt?

Comment 2

[Jeff Walden [:Waldo]]

Interesting; I hadn't seen this before now.  Seems slightly clearer about the purpose than thisDuringConstruction() or whatever, or at least there's a breadcrumb trail to follow to see the why of it, so sounds fine to me.

MOZ_ALLOW_THIS_IN_INITIALIZER_LIST is a bit long, but it *is* pretty clear.  Suggestions for shorter names appreciated.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Introduce MOZ_THIS_IN_INITIALIZER_LIST macro

Unlike chromium's counterpart, this macro has no parameters because the only possible parameter value is "this".
I removed "ALLOW" from the name because the macro itself represents "this".

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 766690 [details] [diff] [review]
Introduce MOZ_THIS_IN_INITIALIZER_LIST macro

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

It'd be good if you fixed up a bunch of existing thisDuringConstruction users to use this, to set a precedent when it lands.

::: mfbt/Attributes.h
@@ +405,5 @@
>  #endif /* MOZ_CLANG_PLUGIN */
>  
> +/*
> + * MOZ_THIS_IN_INITIALIZER_LIST is used to avoid a warning when we know that
> + * it's safe to use 'this' in an initializer list.

Ms2ger and I think this should be a macro function, so that parentheses are required after it.  I'm not quite sure why I think this way, it's mostly an inchoate intuition.  Maybe something about how I might expect an identifier to not have different meanings in different contexts (and obviously |this| differs per class)?  I dunno.  Anyway, smack some parens after this, please.

@@ +408,5 @@
> + * MOZ_THIS_IN_INITIALIZER_LIST is used to avoid a warning when we know that
> + * it's safe to use 'this' in an initializer list.
> + */
> +#ifdef _MSC_VER
> +#define MOZ_THIS_IN_INITIALIZER_LIST \

Indentation nits:

#ifdef _MSC_VER
#  define MOZ... \
     __pragma(...) \
     __pragma(...) \
     this \
     __pragma(...)
#else
#  define ...
#endif

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Introduce MOZ_THIS_IN_INITIALIZER_LIST macro. r=waldo

patch for checkin

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Bug 839998 - Replace thisDuringConstruction() with MOZ_THIS_IN_INITIALIZER_LIST()

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 767297 [details] [diff] [review]
Bug 839998 - Replace thisDuringConstruction() with MOZ_THIS_IN_INITIALIZER_LIST()

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

Just throwing it out there: one nice feature of the Google macro was that you still saw |this| as an identifier in the source, which could be helpful when editing the declaration/definition of particularly large classes, and wanting to know if any part of the constructor uses |this| prematurely.  Might MOZ_IN_INITIALIZER_LIST(this) possibly be better along these lines?  Again, just throwing it out there, not asking for a redo of anything.

Comment 8

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6afe0a48ad5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19ecc13f95a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6afe0a48ad5e
https://hg.mozilla.org/mozilla-central/rev/d19ecc13f95a