Clean up a few includes, introduce iwyu.

Clean up a few includes, introduce iwyu.

The current include-what-you-use with current clang is much less
noisy and more useful than it has been in the past. This change
introduces a few IWYU directives (which are helpful documentation for
humans as well) and fixes a few sets of includes.

Committed: https://skia.googlesource.com/skia/+/f20488b4f2139e6ca09fee7e39b731dd8ab467db

[bungeman-skia, 2015-06-24 21:55:05]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 21:55:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/1

[commit-bot: I haz the power, 2015-06-24 21:56:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 21:56:53]

Dry run: Exceeded global retry quota

[bungeman-skia, 2015-06-24 22:03:14]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 22:03:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/20001

[bungeman-skia, 2015-06-24 22:15:13]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 22:15:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/40001

[bungeman-skia, 2015-06-24 22:42:11]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 22:42:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/60001

[bungeman-skia, 2015-06-24 22:49:16]

bungeman@google.com changed reviewers:
+ mtklein@google.com, reed@google.com

[bungeman-skia, 2015-06-24 22:49:17]

I started working on this because I'm planning on splitting a few really big
files up and it's difficult to tell which includes should go into each of the
new smaller files. Having include-what-you-use just tell me is nice. Also, due
to long history, we have a lot of incorrect includes about. We can clean up the
most obvious incorrect includes manually (assisted by iwyu). If we ever get the
numbers down low enough we could consider some form of automatic check with our
own set of iwyu rules, but I think that's a ways off.

[mtklein, 2015-06-24 22:52:01]

Neat neat neat.

Can you add a script to tools/ or bin/ or something that runs iwyu?

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkRefCnt.h...
include/core/SkRefCnt.h:14: #include "SkInstCnt.h"  // IWYU pragma: export
Does this indicate we should move the contents of SkInstCnt into this file?

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkThread.h
File include/core/SkThread.h (right):

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkThread.h...
include/core/SkThread.h:13: #include "SkAtomics.h"  // IWYU pragma: export
iwyu seems like a good way to clean up this file.  If I'm understanding it
right, we _don't_ want export on these?

[commit-bot: I haz the power, 2015-06-24 22:52:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 22:52:54]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-06-24 23:20:31]

tell me more in person ...

https://codereview.chromium.org/1207893002/diff/60001/src/ports/SkDebug_stdio...
File src/ports/SkDebug_stdio.cpp (right):

https://codereview.chromium.org/1207893002/diff/60001/src/ports/SkDebug_stdio...
src/ports/SkDebug_stdio.cpp:8: #include <stdarg.h>
This one seems odd -- where is the declaration for SkDebugf's signature?

[bungeman-skia, 2015-06-25 14:45:06]

So what I'm currently doing to run this is to build current tip of tree clang
with tip of tree include-what-you-use (the respective websites have very clean
instructions, just use "-G Ninja to build with CMake, it's much faster). Iwyu
runs as a compiler which always fails, so I run it like

CXX=~/bin/clang/bin/include-what-you-use python gyp_skia
ninja -k 0 -C out/Debug/

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkRefCnt.h...
include/core/SkRefCnt.h:14: #include "SkInstCnt.h"  // IWYU pragma: export
On 2015/06/24 22:52:01, mtklein wrote:
> Does this indicate we should move the contents of SkInstCnt into this file?

Maybe? Now that we have asan and the like, is it still worth having it?

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkThread.h
File include/core/SkThread.h (right):

https://codereview.chromium.org/1207893002/diff/60001/include/core/SkThread.h...
include/core/SkThread.h:13: #include "SkAtomics.h"  // IWYU pragma: export
On 2015/06/24 22:52:01, mtklein wrote:
> iwyu seems like a good way to clean up this file.  If I'm understanding it
> right, we _don't_ want export on these?

On the one hand, yes, without these exports iwyu will ask you to include one of
these directly. On the other hand, we already know all uses of this need to be
updated. On the other (third?) hand, having iwyu help clean these up means that
it just happens as we go, and we don't have to guess which one (or both) of
these to include. I'll remove these.

https://codereview.chromium.org/1207893002/diff/60001/src/ports/SkDebug_stdio...
File src/ports/SkDebug_stdio.cpp (right):

https://codereview.chromium.org/1207893002/diff/60001/src/ports/SkDebug_stdio...
src/ports/SkDebug_stdio.cpp:8: #include <stdarg.h>
On 2015/06/24 23:20:31, reed1 wrote:
> This one seems odd -- where is the declaration for SkDebugf's signature?

You're right, iwyu actually though that we didn't want SkTypes.h here because...
it didn't think that this was actually defining anything there because this
wasn't marked as SK_API, even though SK_API is defined to nothing.

This looks like magic, but I think what happens is that it notices that the
declaration has a macro token which expands to "", and the definition must also
have one to match.

This is yet another one of those interesting cases that it points out, but the
thing it suggests is mostly an informed guess. (If it's correct though, it says
it's clean.) As a result, we do need careful scrutiny around fixing includes.

[bungeman-skia, 2015-06-25 15:11:34]

https://codereview.chromium.org/1207893002/diff/80001/src/ports/SkDebug_stdio...
File src/ports/SkDebug_stdio.cpp (right):

https://codereview.chromium.org/1207893002/diff/80001/src/ports/SkDebug_stdio...
src/ports/SkDebug_stdio.cpp:13: SK_API void SkDebugf(const char format[], ...) {
This make iwyu happy, but definitions can't have this, only delarations. I'll
have to see what's really going on.

[bungeman-skia, 2015-06-25 20:38:47]

It's actually rather straight forward (I think) to make iwyu to be more like
what we want, though it does seem to go against its current philosophy.

https://codereview.chromium.org/1207893002/diff/80001/src/ports/SkDebug_stdio...
File src/ports/SkDebug_stdio.cpp (right):

https://codereview.chromium.org/1207893002/diff/80001/src/ports/SkDebug_stdio...
src/ports/SkDebug_stdio.cpp:13: SK_API void SkDebugf(const char format[], ...) {
On 2015/06/25 15:11:34, bungeman1 wrote:
> This make iwyu happy, but definitions can't have this, only delarations. I'll
> have to see what's really going on.

Forget what I said before, this makes iwyu happy because SK_API is in SkTypes.h.
What seems to be really going on is this: iwyu currently makes no distinction
between declarations and definitions. As a result, it will never try to keep a
header just because it declares a function which the current file is defining.
It sometimes seems like it's doing this due to special casing of foo.cc and
foo.h (associated headers), but it actually isn't.

In some sense this is 'correct' in that there is technically no need to forward
declare a function being defined (the forward declaration isn't being 'used').
On the other hand there are warnings in the compiler about having extern
functions without a prior declaration. As a result, I believe iwyu's opinion
here is quite backwards. I would actually prefer that it suggest directly
including at least one header which declares each extern function. In other
words, the forward declaration is a 'use' because any (extern) definition
without a declaration would be an error.

[bungeman-skia, 2015-06-25 21:27:26]

https://codereview.chromium.org/1207893002/diff/80001/src/ports/SkDebug_stdio...
File src/ports/SkDebug_stdio.cpp (right):

https://codereview.chromium.org/1207893002/diff/80001/src/ports/SkDebug_stdio...
src/ports/SkDebug_stdio.cpp:13: SK_API void SkDebugf(const char format[], ...) {
On 2015/06/25 20:38:46, bungeman1 wrote:
> On 2015/06/25 15:11:34, bungeman1 wrote:
> > This make iwyu happy, but definitions can't have this, only delarations.
I'll
> > have to see what's really going on.
> 
> Forget what I said before, this makes iwyu happy because SK_API is in
SkTypes.h.
> What seems to be really going on is this: iwyu currently makes no distinction
> between declarations and definitions. As a result, it will never try to keep a
> header just because it declares a function which the current file is defining.
> It sometimes seems like it's doing this due to special casing of foo.cc and
> foo.h (associated headers), but it actually isn't.
> 
> In some sense this is 'correct' in that there is technically no need to
forward
> declare a function being defined (the forward declaration isn't being 'used').
> On the other hand there are warnings in the compiler about having extern
> functions without a prior declaration. As a result, I believe iwyu's opinion
> here is quite backwards. I would actually prefer that it suggest directly
> including at least one header which declares each extern function. In other
> words, the forward declaration is a 'use' because any (extern) definition
> without a declaration would be an error.

The compiler warning I was referring to turns out to be called
-Wmissing-declarations . This warning does catch removing SkTypes.h from this
file.

[bungeman-skia, 2015-06-25 22:29:13]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-25 22:29:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/100001

[bungeman-skia, 2015-06-25 22:32:25]

So I think I'd like to commit these changes, now that we understand what to
watch out for, and maybe I can bring iwyu around to a saner point of view on
declarations and definitions.

[commit-bot: I haz the power, 2015-06-25 22:35:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-25 22:35:31]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-06-26 14:00:13]

I'm not against this as an experiment, but if you want to really change the skia
code-base for these new conventions, lets have a broader discussion.

lgtm

https://codereview.chromium.org/1207893002/diff/100001/src/ports/SkFontMgr_fo...
File src/ports/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/1207893002/diff/100001/src/ports/SkFontMgr_fo...
src/ports/SkFontMgr_fontconfig.cpp:8: #include "SkFontMgr.h"
This idea of 

special skia headers

"system" headers

other skia headers

Is a new/different convention for Skia. In general, our code has evolved to

skia headers

system headers

If you are suggesting a different pattern, perhaps we can talk about it with the
team.

[bungeman-skia, 2015-06-26 15:01:42]

On 2015/06/26 14:00:13, reed1 wrote:
> I'm not against this as an experiment, but if you want to really change the
skia
> code-base for these new conventions, lets have a broader discussion.
> 
> lgtm
> 
>
https://codereview.chromium.org/1207893002/diff/100001/src/ports/SkFontMgr_fo...
> File src/ports/SkFontMgr_fontconfig.cpp (right):
> 
>
https://codereview.chromium.org/1207893002/diff/100001/src/ports/SkFontMgr_fo...
> src/ports/SkFontMgr_fontconfig.cpp:8: #include "SkFontMgr.h"
> This idea of 
> 
> special skia headers
> 
> "system" headers
> 
> other skia headers
> 
> Is a new/different convention for Skia. In general, our code has evolved to
> 
> skia headers
> 
> system headers
> 
> If you are suggesting a different pattern, perhaps we can talk about it with
the
> team.

Just wanted to comment the rationale for this order at
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...
. The idea that the "associated" header (foo.cc has associated header foo.h, in
other words, the header which has the declarations this implementation file is
defining) should go first has a good rationale, which is that every header
should be included first at some point to ensure that it can be included alone.
This means that if you make changes to the associated header which prevent it
from being included alone, it's your build which breaks first (when you build
with that header change) instead of some other project which uses your headers
(in a different order). There is no good rationale for the rest of the order, as
far as I can tell.

Thinking about it, one would generally want to go from most specific to most
general (user configuration header, associated header, project headers, platform
headers, language headers) on the idea that each of these can only rely on
earlier ones. Of course, at our level there is no real distinction between
platform and language headers. ... and I've just re-invented the WebKit style
guide's include order https://www.webkit.org/coding/coding-style.html#include .
If we also had Cary's "compile each header independently" system set up as a
build then there's no reason left to have the associated header come first. We
also want one thing backward from WebKit, which is that I think it is desired
that SkTypes.h appear first but only in headers, but never in an implementation
file (WebKit puts config.h in implementation files but never in headers). I
think the difference is that WebKit wants the interface to always be the same
and for config.h to only affect implementation details, where Skia actually does
use the configuration to affect the interface.

So I think what is desired is a way to compile every header file independently
as a build verification step; list headers as Skia headers, then system headers;
and only include SkTypes.h in headers, only allowing it in implementation files
if it's the only include. Is that what is desired? We should add an 'includes'
section to https://skia.org/dev/contrib/style .

[mtklein, 2015-06-26 15:24:32]

On 2015/06/26 15:01:42, bungeman1 wrote:
> On 2015/06/26 14:00:13, reed1 wrote:
> > I'm not against this as an experiment, but if you want to really change the
> skia
> > code-base for these new conventions, lets have a broader discussion.
> > 
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1207893002/diff/100001/src/ports/SkFontMgr_fo...
> > File src/ports/SkFontMgr_fontconfig.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1207893002/diff/100001/src/ports/SkFontMgr_fo...
> > src/ports/SkFontMgr_fontconfig.cpp:8: #include "SkFontMgr.h"
> > This idea of 
> > 
> > special skia headers
> > 
> > "system" headers
> > 
> > other skia headers
> > 
> > Is a new/different convention for Skia. In general, our code has evolved to
> > 
> > skia headers
> > 
> > system headers
> > 
> > If you are suggesting a different pattern, perhaps we can talk about it with
> the
> > team.
> 
> Just wanted to comment the rationale for this order at
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...
> . The idea that the "associated" header (foo.cc has associated header foo.h,
in
> other words, the header which has the declarations this implementation file is
> defining) should go first has a good rationale, which is that every header
> should be included first at some point to ensure that it can be included
alone.
> This means that if you make changes to the associated header which prevent it
> from being included alone, it's your build which breaks first (when you build
> with that header change) instead of some other project which uses your headers
> (in a different order). There is no good rationale for the rest of the order,
as
> far as I can tell.
> 
> Thinking about it, one would generally want to go from most specific to most
> general (user configuration header, associated header, project headers,
platform
> headers, language headers) on the idea that each of these can only rely on
> earlier ones. Of course, at our level there is no real distinction between
> platform and language headers. ... and I've just re-invented the WebKit style
> guide's include order https://www.webkit.org/coding/coding-style.html#include
.
> If we also had Cary's "compile each header independently" system set up as a
> build then there's no reason left to have the associated header come first. We
> also want one thing backward from WebKit, which is that I think it is desired
> that SkTypes.h appear first but only in headers, but never in an
implementation
> file (WebKit puts config.h in implementation files but never in headers). I
> think the difference is that WebKit wants the interface to always be the same
> and for config.h to only affect implementation details, where Skia actually
does
> use the configuration to affect the interface.
> 
> So I think what is desired is a way to compile every header file independently
> as a build verification step; list headers as Skia headers, then system
headers;
> and only include SkTypes.h in headers, only allowing it in implementation
files
> if it's the only include. Is that what is desired? We should add an 'includes'
> section to https://skia.org/dev/contrib/style .

I would like our #include style guide to read: try to keep all the #includes
together up at the top of the file and keep them tidy, preferably sorted.

[bungeman-skia, 2015-07-28 19:34:58]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 19:35:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207893002/140001

[commit-bot: I haz the power, 2015-07-28 19:36:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-28 19:36:04]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[bungeman-skia, 2015-07-28 19:53:09]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 19:53:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207893002/160001

[commit-bot: I haz the power, 2015-07-28 19:54:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-28 19:54:09]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[bungeman-skia, 2015-07-28 19:59:39]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 19:59:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207893002/180001

[commit-bot: I haz the power, 2015-07-28 20:00:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-28 20:00:46]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[bungeman-skia, 2015-07-28 20:12:05]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 20:12:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207893002/200001

[commit-bot: I haz the power, 2015-07-28 20:20:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-28 20:20:03]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2015-07-28 21:20:32]

At Patch Set 11, perhaps this is a little more straight forward and less
controversial. This cleans up most of the ports target.

https://codereview.chromium.org/1207893002/diff/120001/src/ports/SkDebug_stdi...
File src/ports/SkDebug_stdio.cpp (right):

https://codereview.chromium.org/1207893002/diff/120001/src/ports/SkDebug_stdi...
src/ports/SkDebug_stdio.cpp:13: void SkDebugf(const char format[], ...) {
With a local change to fix
https://github.com/include-what-you-use/include-what-you-use/issues/179 (a
definition of a global function is a use of previous declarations) iwyu no
longer thinks this definition no longer needs the declaration in SkTypes. This
also quiets a number of other 'false positives'. Hopefully this local change can
be cleaned up and up-streamed.

https://codereview.chromium.org/1207893002/diff/140001/include/core/SkPostCon...
File include/core/SkPostConfig.h (left):

https://codereview.chromium.org/1207893002/diff/140001/include/core/SkPostCon...
include/core/SkPostConfig.h:219: #  include <string.h>
This was somewhat magically providing size_t, NULL, and memset to everything
else. Since we don't need this conditional anymore, move the include out to
SkTypes.

https://codereview.chromium.org/1207893002/diff/140001/include/core/SkPostCon...
include/core/SkPostConfig.h:220: #  include <stdlib.h>
Nothing in SkTypes needs this, and most files don't, but at least
SkGraphics.cpp, SkParse.cpp, SkRTCont.cpp, SkResourceCache.cpp, and
SkRegion_path.cpp (for qsort!?) currently depend on this being here to provide
various declarations.

[mtklein, 2015-07-28 22:04:19]

Sorry, I realize I both marked a bunch of redundant stddef.h questions and left
off some too...

https://codereview.chromium.org/1207893002/diff/200001/include/core/SkTypes.h
File include/core/SkTypes.h (right):

https://codereview.chromium.org/1207893002/diff/200001/include/core/SkTypes.h...
include/core/SkTypes.h:15: #include <stdint.h>
Should we just shove <stddef.h> in here too so we unambiguously get size_t +
NULL?

https://codereview.chromium.org/1207893002/diff/200001/include/ports/SkFontMg...
File include/ports/SkFontMgr_indirect.h (right):

https://codereview.chromium.org/1207893002/diff/200001/include/ports/SkFontMg...
include/ports/SkFontMgr_indirect.h:20: #include <stddef.h>
I think I like SkTypes providing size_t.  Is that what this is for?

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkDiscardabl...
File src/ports/SkDiscardableMemory_none.cpp (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkDiscardabl...
src/ports/SkDiscardableMemory_none.cpp:11: #include <stddef.h>
I think I'd rather "SkTypes.h" than <stddef.h>, unless we switch everything over
to include <stdfoo.h> themselves.

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
File src/ports/SkFontMgr_android.cpp (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
src/ports/SkFontMgr_android.cpp:26: #include <stdlib.h>
What triggered this?  Do we have a stray malloc or free or something?

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
File src/ports/SkFontMgr_android_parser.h (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
src/ports/SkFontMgr_android_parser.h:19: #include <cstddef>
Can we get this from SkTypes?

[bungeman-skia, 2015-07-28 22:48:10]

I suppose stddef.h isn't too big. Personally I dislike all this forced inclusion
of headers on those who aren't using them.

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
File src/ports/SkFontMgr_android.cpp (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
src/ports/SkFontMgr_android.cpp:26: #include <stdlib.h>
On 2015/07/28 22:04:19, mtklein wrote:
> What triggered this?  Do we have a stray malloc or free or something?

NULL, abs, size_t

We should probably use SkTAbs instead of abs.

[reed1, 2015-07-29 14:40:39]

https://codereview.chromium.org/1207893002/diff/200001/include/core/SkTypes.h
File include/core/SkTypes.h (right):

https://codereview.chromium.org/1207893002/diff/200001/include/core/SkTypes.h...
include/core/SkTypes.h:25: #include <stdlib.h>
just curious: what do these two bring us that we use?

[bungeman-skia, 2015-07-29 16:46:23]

Added stddef to SkTypes exports, cleaned up for that. See more comments below.

https://codereview.chromium.org/1207893002/diff/200001/include/core/SkTypes.h
File include/core/SkTypes.h (right):

https://codereview.chromium.org/1207893002/diff/200001/include/core/SkTypes.h...
include/core/SkTypes.h:15: #include <stdint.h>
On 2015/07/28 22:04:19, mtklein wrote:
> Should we just shove <stddef.h> in here too so we unambiguously get size_t +
> NULL?

Done.

https://codereview.chromium.org/1207893002/diff/200001/include/core/SkTypes.h...
include/core/SkTypes.h:25: #include <stdlib.h>
On 2015/07/29 14:40:39, reed1 wrote:
> just curious: what do these two bring us that we use?

stdlib: Nothing in SkTypes needs this, and most files don't, but at least
SkGraphics.cpp, SkParse.cpp, SkRTCont.cpp, SkResourceCache.cpp, and
SkRegion_path.cpp (for qsort!?) currently depend on this being here to provide
various declarations.

string.h: memset below, and I think some files are depending on it for various
things, I haven't looked too hard at this one yet.

https://codereview.chromium.org/1207893002/diff/200001/include/ports/SkFontMg...
File include/ports/SkFontMgr_indirect.h (right):

https://codereview.chromium.org/1207893002/diff/200001/include/ports/SkFontMg...
include/ports/SkFontMgr_indirect.h:20: #include <stddef.h>
On 2015/07/28 22:04:19, mtklein wrote:
> I think I like SkTypes providing size_t.  Is that what this is for?

It isn't! Well, it was by accident by including string.h (in SkPostConfig.h).
Will make SkTypes provide this explicitly through stddef.h (which is much
smaller and saner than other headers).

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkDiscardabl...
File src/ports/SkDiscardableMemory_none.cpp (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkDiscardabl...
src/ports/SkDiscardableMemory_none.cpp:11: #include <stddef.h>
On 2015/07/28 22:04:19, mtklein wrote:
> I think I'd rather "SkTypes.h" than <stddef.h>, unless we switch everything
over
> to include <stdfoo.h> themselves.

I'd like to switch to <cstdfoo> directly. I assume the reason for any of the
standard headers being 'provided' by SkTypes is that in the wild west of the
past you may not actually have them, so it might need to synthesize them, or
include something else (see history of stdint for a good example). It's fine for
SkTypes to include whatever headers it needs for it's own code (which there
seems to excessive amounts of, some of it needs to be in SkTemplates.h. and that
might want to be broken up a bit too), but I'd like to minimize the amount it
purposefully exports (for example stdlib is really big).

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
File src/ports/SkFontMgr_android_parser.h (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_an...
src/ports/SkFontMgr_android_parser.h:19: #include <cstddef>
On 2015/07/28 22:04:19, mtklein wrote:
> Can we get this from SkTypes?

Done.

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_cu...
File src/ports/SkFontMgr_custom.cpp (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_cu...
src/ports/SkFontMgr_custom.cpp:23: #include <stddef.h>
And this one.

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_fo...
File src/ports/SkFontMgr_fontconfig_factory.cpp (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkFontMgr_fo...
src/ports/SkFontMgr_fontconfig_factory.cpp:11: #include <stddef.h>
And this one.

[bungeman-skia, 2015-07-29 17:30:22]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-29 17:30:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207893002/220001

[commit-bot: I haz the power, 2015-07-29 17:38:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-29 17:38:20]

Dry run: This issue passed the CQ dry run.

[mtklein, 2015-07-29 18:05:02]

lgtm

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkDiscardabl...
File src/ports/SkDiscardableMemory_none.cpp (right):

https://codereview.chromium.org/1207893002/diff/200001/src/ports/SkDiscardabl...
src/ports/SkDiscardableMemory_none.cpp:11: #include <stddef.h>
On 2015/07/29 16:46:23, bungeman1 wrote:
> On 2015/07/28 22:04:19, mtklein wrote:
> > I think I'd rather "SkTypes.h" than <stddef.h>, unless we switch everything
> over
> > to include <stdfoo.h> themselves.
> 
> I'd like to switch to <cstdfoo> directly. I assume the reason for any of the
> standard headers being 'provided' by SkTypes is that in the wild west of the
> past you may not actually have them, so it might need to synthesize them, or
> include something else (see history of stdint for a good example). It's fine
for
> SkTypes to include whatever headers it needs for it's own code (which there
> seems to excessive amounts of, some of it needs to be in SkTemplates.h. and
that
> might want to be broken up a bit too), but I'd like to minimize the amount it
> purposefully exports (for example stdlib is really big).

sgtm

[bungeman-skia, 2015-07-29 18:48:45]

The CQ bit was checked by bungeman@google.com

[bungeman-skia, 2015-07-29 18:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1207893002/#ps220001
(title: "SkTypes to export stddef.")

[commit-bot: I haz the power, 2015-07-29 18:49:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207893002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207893002/220001

[commit-bot: I haz the power, 2015-07-29 18:49:43]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/f20488b4f2139e6ca09fee7e39b731dd8ab467db