Prefer C++ standard headers to their C counterpart #21091
Conversation
cbracken
requested review from
chinmaygarde,
jason-simmons and
stuartmorgan-g
|
|
||
| #include "flutter/fml/build_config.h" | ||
|
|
||
| #if defined(OS_WIN) |
There was a problem hiding this comment.
What is this change for?
There was a problem hiding this comment.
This symbol is only required on Windows (for M_PI etc.) and this is the only place we actually wrap it in a platform check, but also the place we benefit least from doing so (unittest). We could wrap it everywhere, but figured it would be nice to keep the code consistent so people don't end up doing a mix of the two approaches.
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #define _USE_MATH_DEFINES |
There was a problem hiding this comment.
Why not keep this with the header?
(Applies to the same change in other files as well.)
There was a problem hiding this comment.
In a bunch of cases, we pull in cmath without defining this; if the first include is via something transitively included via canvas.h, we end up getting it without M_PI etc.
There are a couple options: one is that everywhere we include cmath, we define this. Another is the approach of defining this symbol before the first include in a given file, which feels safer, despite the fact it would be nice to keep them side-by-side. Thoughts/preferences?
There was a problem hiding this comment.
Could we add it as a define in GN instead?
There was a problem hiding this comment.
That's a good point. Will take a look; that avoids cluttering the code with this altogether. It's not obvious to readers at first blush why this is there until you look up what the define does and it's not anything insightful when you do finally find out.
cbracken
added
Work in progress (WIP)
We currently use a mix of C standard includes (e.g. limits.h) and their C++ variants (e.g. climits). This migrates to a consistent style for all cases where the C++ variants are acceptable, but leaves the C equivalents in place where they are required, such as in the embedder API and other headers that may be used from C.
3 participants
We currently use a mix of C standard includes (e.g. limits.h) and their
C++ variants (e.g. climits). This migrates to a consistent style for all
cases where the C++ variants are acceptable, but leaves the C
equivalents in place where they are required, such as in the embedder
API and other headers that may be used from C.