Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Build warning: m_jsThread will be initialized after m_delegate #18384

Closed
fbartho opened this issue Mar 14, 2018 · 2 comments
Closed

Build warning: m_jsThread will be initialized after m_delegate #18384

fbartho opened this issue Mar 14, 2018 · 2 comments
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Platform: iOS iOS applications. Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot. Resolution: PR Submitted A pull request with a fix has been provided.

Comments

@fbartho
Copy link
Contributor

fbartho commented Mar 14, 2018 •
edited
Loading

When debugging react-native iOS, and it crashes (for unrelated reasons), it drops me in RCTObjcExecutor and metaphorically stabs me in the eye with a build warning:
Field 'm_jsThread' will be initialized after field 'm_delegate'

I believe this is simply an issue with initializer list ordering vs private member-variable ordering, and I'll be submitting a PR to fix it.

Environment

$ react-native info
Environment:
  OS: macOS High Sierra 10.13.3
  Node: 9.7.1
  Yarn: 1.5.1
  npm: 5.6.0
  Watchman: 4.9.0
  Xcode: Xcode 9.2 Build version 9C40b
  Android Studio: 3.0 AI-171.4443003

Packages: (wanted => installed)
  react: 16.2.x => 16.2.0
  react-native: 0.54.x => 0.54.2

Expected Behavior

react-native should strive to minimize the build warnings that occur with normal use.

Actual Behavior

screen shot 2018-03-14 at 1 37 23 pm

Warning documented above is thrown on these lines:

react-native/React/CxxBridge/RCTObjcExecutor.mm

Lines 42 to 43 in 0d03fdd

, m_jsThread(std::move(jsThread))
, m_delegate(std::move(delegate))

Steps to Reproduce

Go to RCTObjExecutor.mm Line 42 & notice the build warning that Xcode shows you.
@react-native-bot
Copy link
Collaborator

Thanks for posting this! It looks like your issue may refer to an older version of React Native. Can you reproduce the issue on the latest stable release?

Thank you for your contributions.

How to Contribute • What to Expect from Maintainers

@react-native-bot react-native-bot added Old Version ⏪ Ran Commands One of our bots successfully processed a command. labels Mar 14, 2018
@fbartho
Copy link
Contributor Author

fbartho commented Mar 14, 2018

Yes, I've updated the Environment to show that it is reproducible on 0.54.2

fbartho added a commit to fbartho/react-native that referenced this issue Mar 14, 2018
@fbartho
RCTObjcExecutor: fix initializer list order initialization warning.
a034427 
Fixes: facebook#18384
@fbartho fbartho mentioned this issue Mar 14, 2018
Closed
@hramos hramos added Platform: iOS iOS applications. Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Resolution: PR Submitted A pull request with a fix has been provided. and removed Old Version ⏪ labels Mar 14, 2018
hamaron pushed a commit to hamaron/react-native that referenced this issue Apr 9, 2018
@fbartho @hamaron
RCTObjcExecutor: fix initializer-list initialization order build warn…
467cc18 
…ing.

Summary:
Fixes: facebook#18384

Previously line 42 had a warning of: `Field 'm_jsThread' will be initialized after field 'm_delegate'`

This appears to be because the private member fields are declared in a different order than the initializer-list was told to initialize things.

I chose to re-order the initializer list because that fixes the warning by matching the code structure to what the compiler was telling us it was going to do.

An alternate fix would be to change the order of the member variables declared in the `private:` section to match the coded order in the initializer list. This might be the right move if the initializer list had the correct intent, but this would be a breaking change from a behavior perspective, so I'd need somebody with more knowledge to suggest this is the right move.

A. Examine line to see the warning is gone.
B. Get a C++ guru to figure out the intent of this file and see if this actually exposed a bug that recommends we go with the alternate fix suggested above.

[IOS][MINOR][React/CxxBridge/] - Build warning fix

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook#18385

Differential Revision: D7328272

Pulled By: mhorowitz

fbshipit-source-id: 59d7ace6222c4609ac7d023f97e522e85e02a9db
campsafari pushed a commit to exozet/react-native that referenced this issue Apr 11, 2018
@fbartho @campsafari
RCTObjcExecutor: fix initializer-list initialization order build warn…
bbe02ab 
…ing.

Summary:
Fixes: facebook#18384

Previously line 42 had a warning of: `Field 'm_jsThread' will be initialized after field 'm_delegate'`

This appears to be because the private member fields are declared in a different order than the initializer-list was told to initialize things.

I chose to re-order the initializer list because that fixes the warning by matching the code structure to what the compiler was telling us it was going to do.

An alternate fix would be to change the order of the member variables declared in the `private:` section to match the coded order in the initializer list. This might be the right move if the initializer list had the correct intent, but this would be a breaking change from a behavior perspective, so I'd need somebody with more knowledge to suggest this is the right move.

A. Examine line to see the warning is gone.
B. Get a C++ guru to figure out the intent of this file and see if this actually exposed a bug that recommends we go with the alternate fix suggested above.

[IOS][MINOR][React/CxxBridge/] - Build warning fix

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook#18385

Differential Revision: D7328272

Pulled By: mhorowitz

fbshipit-source-id: 59d7ace6222c4609ac7d023f97e522e85e02a9db
LukeDurrant pushed a commit to LukeDurrant/react-native that referenced this issue Apr 11, 2018
@fbartho @LukeDurrant
RCTObjcExecutor: fix initializer-list initialization order build warn…
3ef4f7d 
…ing.

Summary:
Fixes: facebook#18384

Previously line 42 had a warning of: `Field 'm_jsThread' will be initialized after field 'm_delegate'`

This appears to be because the private member fields are declared in a different order than the initializer-list was told to initialize things.

I chose to re-order the initializer list because that fixes the warning by matching the code structure to what the compiler was telling us it was going to do.

An alternate fix would be to change the order of the member variables declared in the `private:` section to match the coded order in the initializer list. This might be the right move if the initializer list had the correct intent, but this would be a breaking change from a behavior perspective, so I'd need somebody with more knowledge to suggest this is the right move.

A. Examine line to see the warning is gone.
B. Get a C++ guru to figure out the intent of this file and see if this actually exposed a bug that recommends we go with the alternate fix suggested above.

[IOS][MINOR][React/CxxBridge/] - Build warning fix

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook#18385

Differential Revision: D7328272

Pulled By: mhorowitz

fbshipit-source-id: 59d7ace6222c4609ac7d023f97e522e85e02a9db
LukeDurrant pushed a commit to LukeDurrant/react-native that referenced this issue Apr 11, 2018
@fbartho @LukeDurrant
RCTObjcExecutor: fix initializer-list initialization order build warn…
023a18c 
…ing.

Summary:
Fixes: facebook#18384

Previously line 42 had a warning of: `Field 'm_jsThread' will be initialized after field 'm_delegate'`

This appears to be because the private member fields are declared in a different order than the initializer-list was told to initialize things.

I chose to re-order the initializer list because that fixes the warning by matching the code structure to what the compiler was telling us it was going to do.

An alternate fix would be to change the order of the member variables declared in the `private:` section to match the coded order in the initializer list. This might be the right move if the initializer list had the correct intent, but this would be a breaking change from a behavior perspective, so I'd need somebody with more knowledge to suggest this is the right move.

A. Examine line to see the warning is gone.
B. Get a C++ guru to figure out the intent of this file and see if this actually exposed a bug that recommends we go with the alternate fix suggested above.

[IOS][MINOR][React/CxxBridge/] - Build warning fix

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook#18385

Differential Revision: D7328272

Pulled By: mhorowitz

fbshipit-source-id: 59d7ace6222c4609ac7d023f97e522e85e02a9db
bunnyc1986 pushed a commit to bunnyc1986/react-native that referenced this issue May 11, 2018
@fbartho @bunnyc1986
RCTObjcExecutor: fix initializer-list initialization order build warn…
fbc065a 
…ing.

Summary:
Fixes: facebook#18384

Previously line 42 had a warning of: `Field 'm_jsThread' will be initialized after field 'm_delegate'`

This appears to be because the private member fields are declared in a different order than the initializer-list was told to initialize things.

I chose to re-order the initializer list because that fixes the warning by matching the code structure to what the compiler was telling us it was going to do.

An alternate fix would be to change the order of the member variables declared in the `private:` section to match the coded order in the initializer list. This might be the right move if the initializer list had the correct intent, but this would be a breaking change from a behavior perspective, so I'd need somebody with more knowledge to suggest this is the right move.

A. Examine line to see the warning is gone.
B. Get a C++ guru to figure out the intent of this file and see if this actually exposed a bug that recommends we go with the alternate fix suggested above.

[IOS][MINOR][React/CxxBridge/] - Build warning fix

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook#18385

Differential Revision: D7328272

Pulled By: mhorowitz

fbshipit-source-id: 59d7ace6222c4609ac7d023f97e522e85e02a9db
macdoum1 pushed a commit to macdoum1/react-native that referenced this issue Jun 28, 2018
@fbartho @macdoum1
RCTObjcExecutor: fix initializer-list initialization order build warn…
be99145 
…ing.

Summary:
Fixes: facebook#18384

Previously line 42 had a warning of: `Field 'm_jsThread' will be initialized after field 'm_delegate'`

This appears to be because the private member fields are declared in a different order than the initializer-list was told to initialize things.

I chose to re-order the initializer list because that fixes the warning by matching the code structure to what the compiler was telling us it was going to do.

An alternate fix would be to change the order of the member variables declared in the `private:` section to match the coded order in the initializer list. This might be the right move if the initializer list had the correct intent, but this would be a breaking change from a behavior perspective, so I'd need somebody with more knowledge to suggest this is the right move.

A. Examine line to see the warning is gone.
B. Get a C++ guru to figure out the intent of this file and see if this actually exposed a bug that recommends we go with the alternate fix suggested above.

[IOS][MINOR][React/CxxBridge/] - Build warning fix

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook#18385

Differential Revision: D7328272

Pulled By: mhorowitz

fbshipit-source-id: 59d7ace6222c4609ac7d023f97e522e85e02a9db
@facebook facebook locked as resolved and limited conversation to collaborators Mar 19, 2019
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Mar 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Platform: iOS iOS applications. Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot. Resolution: PR Submitted A pull request with a fix has been provided.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

RCTObjcExecutor: fix initializer-list initialization order build warning. fbartho/react-native
3 participants
@hramos @fbartho @react-native-bot