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

Conversation

@cbracken
Copy link
Member

On Windows, to enable constants like M_PI in cmath/math.h the symbol
_USE_MATH_DEFINES must be defined. It's easy to forget, and needs to be
defined before the first (transitive) import of cmath, so instead we
define in the build targets in which it's used.

@cbracken
Copy link
Member Author

Is there a single top-level place we could define this so people don't need to remember to add it if we start using Pi in a new place in the code? Realistically, I suspect we're unlikely to use it anywhere other than lib/ui.

@stuartmorgan-g
Copy link
Contributor

Is there a single top-level place we could define this so people don't need to remember to add it if we start using Pi in a new place in the code?

We could put in in the global configs in the buildroot, but we probably don't want to do that; it would cause duplicate definition warnings if any third-party code defined it in their source.

lib/ui/BUILD.gn Outdated
"//third_party/skia",
]

defines = []
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dangerous; if a config set defines, this could stomp them. Setting an existing list to something else is normally an error in GN, but if you set it to an empty list it assumes you're explicitly trying to clear it (https://gn.googlesource.com/gn/+/master/docs/reference.md#lists). I hit a case in Dart code where this pattern was accidentally suppressing what was intended to be a widely applied config.

I think what you want is:

if (!defined(defines)) {
  defines = []
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. Done!

On Windows, to enable constants like M_PI in cmath/math.h the symbol
_USE_MATH_DEFINES must be defined. It's easy to forget, and needs to be
defined before the first (transitive) import of cmath, so instead we
define in the build targets in which it's used.
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 15, 2020
@fluttergithubbot fluttergithubbot merged commit 2a26bfb into flutter:master Sep 15, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 15, 2020
@cbracken cbracken deleted the windows_math_defs branch May 10, 2022 21:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants