Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Move constant definitions out embedder.h #8498

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

stuartmorgan-g
Copy link
Contributor

Constant values should generally not be defined in headers. As currently written, multiple files including this header will cause link errors as the symbols are redeclared in each file with external linkage.

@stuartmorgan-g
Copy link
Contributor Author

(If for some reason the value absolutely must be in the header, we probably want a #define; declaring them static would address the link error, but is potentially dangerous in some edge cases.)

@jamesderlin
Copy link
Contributor

(If for some reason the value absolutely must be in the header, we probably want a #define; declaring them static would address the link error, but is potentially dangerous in some edge cases.)

Or use enum.

@GeertJohan
Copy link

Since the value itself is part of the API "contract", it may make sense to use enum like other kValues in the header file. If that's possible, since the enum value must still be declared -1 to be compatible?

This would then make it easy to use the value while only including the embedder.h file.

@stuartmorgan-g
Copy link
Contributor Author

Since the value itself is part of the API "contract"

Chris will need to weigh in on that; it's not clear to me that it is.

@cbracken
Copy link
Member

cbracken commented Apr 10, 2019

There's no reason at all for the value itself to be in the header; these should definitely have been declared extern initially. Good catch!

The value itself must be < 0. The choice of -1 was arbitrary within that constraint. That said, we should preserve that choice in order to ensure compatibility without forcing a recompile of embedders.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll need a rebase to head to pass presubmit. Other than that, lgtm.

lgtm_hanko

@stuartmorgan-g stuartmorgan-g merged commit 72986c3 into flutter:master Apr 10, 2019
@stuartmorgan-g stuartmorgan-g deleted the embedder-constants-fix branch April 10, 2019 17:03
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 10, 2019
flutter/engine@905c571...72986c3

git log 905c571..72986c3 --no-merges --oneline
72986c3 Move constant definitions out embedder.h (flutter/engine#8498)
e356dbc Merge flutter/synchronization contents into fml. (flutter/engine#8525)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff (jsimmons@google.com), and stop
the roller if necessary.
@chinmaygarde
Copy link
Member

Can you add a check so that this sort of thing gets caught at build time? We have an embedder_include.c that acts as a sanity check to make sure the header can be included in a C context. I suggest adding an embedder_include2.c and preparing an executable target that gets built in the default rules. The linker will trip on errors like this.

stuartmorgan-g added a commit to stuartmorgan-g/engine that referenced this pull request Apr 11, 2019
PR flutter#8498 made these constants extern, but forgot to export them so they
would be public symbols.
stuartmorgan-g pushed a commit that referenced this pull request Apr 12, 2019
PR #8498 made these constants extern, but forgot to export them so they
would be public symbols.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants