Guard against most unintentionally ephemeral SkAutoFoo instantiations.

Guard against most unintentionally ephemeral SkAutoFoo instantiations.

I think I applied the trick everywhere possible.  Limitations:
    - can't be used with templated classes
    - all constructors and destructors must be defined inline

A couple of the SkAutoFoo were unused in Skia, Chromium, and Android, so I
deleted them.  This change caught the same bugs Cary found in SkPath, plus one
more in SampleApp.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12301

[caryclark, 2013-11-14 16:50:33]

lgtm -- my change to add fix SkPath.cpp is winding its way through the commit
queue process to be extra-safe in these mobile times -- will this conflict?

[reed1, 2013-11-14 18:30:51]

can we have a macro that creates this macro?

e.g.

DECLARE_GUARD_CONSTRUCTOR(myclass)

[mtklein, 2013-11-14 18:53:34]

On 2013/11/14 18:30:51, reed1 wrote:
> can we have a macro that creates this macro?
> 
> e.g.
> 
> DECLARE_GUARD_CONSTRUCTOR(myclass)

Hmm.  To my knowledge, we can't.  The hitch is that we'd be trying to do a
#define inside a #define.

[bungeman-skia, 2013-11-14 20:56:06]

Is it possible to use variadic macros to do this (which would be a nice thing to
do on compilers which support it, which is most of them). Of course, all
compilers might not support it, if so we'd need some way to abstract this as
reed suggested, though I have no idea how to do so.

[mtklein, 2013-11-15 14:33:34]

On 2013/11/14 20:56:06, bungeman1 wrote:
> Is it possible to use variadic macros to do this (which would be a nice thing
to
> do on compilers which support it, which is most of them). Of course, all
> compilers might not support it, if so we'd need some way to abstract this as
> reed suggested, though I have no idea how to do so.

How would that look?  Don't you still have the problem of the preprocessor being
a single pass?

#define FOO(...) #define ...

[bungeman-skia, 2013-11-15 15:15:50]

On 2013/11/15 14:33:34, mtklein wrote:
> On 2013/11/14 20:56:06, bungeman1 wrote:
> > Is it possible to use variadic macros to do this (which would be a nice
thing
> to
> > do on compilers which support it, which is most of them). Of course, all
> > compilers might not support it, if so we'd need some way to abstract this as
> > reed suggested, though I have no idea how to do so.
> 
> How would that look?  Don't you still have the problem of the preprocessor
being
> a single pass?
> 
> #define FOO(...) #define ...

Ah, I meant to avoid the "which one do I protect" issue. Like

#define SkAutoDisableDirectionCheck(...)

I have no idea how one might trick the identifier in "#define <identifier>" into
being a computed thing.

[mtklein, 2013-11-15 15:26:46]

On 2013/11/15 15:15:50, bungeman1 wrote:
> On 2013/11/15 14:33:34, mtklein wrote:
> > On 2013/11/14 20:56:06, bungeman1 wrote:
> > > Is it possible to use variadic macros to do this (which would be a nice
> thing
> > to
> > > do on compilers which support it, which is most of them). Of course, all
> > > compilers might not support it, if so we'd need some way to abstract this
as
> > > reed suggested, though I have no idea how to do so.
> > 
> > How would that look?  Don't you still have the problem of the preprocessor
> being
> > a single pass?
> > 
> > #define FOO(...) #define ...
> 
> Ah, I meant to avoid the "which one do I protect" issue. Like
> 
> #define SkAutoDisableDirectionCheck(...)
> 
> I have no idea how one might trick the identifier in "#define <identifier>"
into
> being a computed thing.

Ah!  This is a good idea.  Variadic macros are pretty nearly universally
supported.  I'll follow this up with a patch to switch to (...), separate from
this in case I have to roll it back.

Before I figured out it wasn't possible, I was feeling clever and thought, "Oh
maybe we can just pull the #define outside the expression," like this,
  #define GUARDED_AUTO(classname) classname(...) SK_COMPILE_ASSERT(false,
missing_name_for_##classname)
then use it like
  #define GUARDED_AUTO(SkAutoFoo)
but, of course, that just fails while attempting to redefine GUARDED_AUTO.  :(

[bungeman-skia, 2013-11-15 15:38:16]

On 2013/11/15 15:26:46, mtklein wrote:
> On 2013/11/15 15:15:50, bungeman1 wrote:
> > On 2013/11/15 14:33:34, mtklein wrote:
> > > On 2013/11/14 20:56:06, bungeman1 wrote:
> > > > Is it possible to use variadic macros to do this (which would be a nice
> > thing
> > > to
> > > > do on compilers which support it, which is most of them). Of course, all
> > > > compilers might not support it, if so we'd need some way to abstract
this
> as
> > > > reed suggested, though I have no idea how to do so.
> > > 
> > > How would that look?  Don't you still have the problem of the preprocessor
> > being
> > > a single pass?
> > > 
> > > #define FOO(...) #define ...
> > 
> > Ah, I meant to avoid the "which one do I protect" issue. Like
> > 
> > #define SkAutoDisableDirectionCheck(...)
> > 
> > I have no idea how one might trick the identifier in "#define <identifier>"
> into
> > being a computed thing.
> 
> Ah!  This is a good idea.  Variadic macros are pretty nearly universally
> supported.  I'll follow this up with a patch to switch to (...), separate from
> this in case I have to roll it back.
> 
> Before I figured out it wasn't possible, I was feeling clever and thought, "Oh
> maybe we can just pull the #define outside the expression," like this,
>   #define GUARDED_AUTO(classname) classname(...) SK_COMPILE_ASSERT(false,
> missing_name_for_##classname)
> then use it like
>   #define GUARDED_AUTO(SkAutoFoo)
> but, of course, that just fails while attempting to redefine GUARDED_AUTO.  :(

Yeah, I was just looking at the C spec, and there isn't any macro expansion on
the <identifier>. On the other hand, I think this would be clearer (at least
show some intent / documentation) by being like

in SkTypes.h

// SkAutoFoo values should never be created without a name.
#define SK_DISABLE_RVALUE_CONSTRUCTOR(classname) SK_COMPILE_ASSERT(false,
rvalue_constuction_disabled_for_##classname)

after declaration

#define SkAutoDisableDirectionCheck(...)
SK_DISABLE_RVALUE_CONSTRUCTOR(SkAutoDisableDirectionCheck)

So at least we have something a bit more explanatory at the declaration site. At
the moment you just have to know what this is there for. I had a bit of trouble
remembering why we were doing this when I first looked at the CL, and I was
there when it was being discussed. This is still rather repeating itself, but
due to no macro expansion available here, this might be the best one can do with
what we have.

[mtklein, 2013-11-15 17:19:40]

On 2013/11/15 15:38:16, bungeman1 wrote:
> On 2013/11/15 15:26:46, mtklein wrote:
> > On 2013/11/15 15:15:50, bungeman1 wrote:
> > > On 2013/11/15 14:33:34, mtklein wrote:
> > > > On 2013/11/14 20:56:06, bungeman1 wrote:
> > > > > Is it possible to use variadic macros to do this (which would be a
nice
> > > thing
> > > > to
> > > > > do on compilers which support it, which is most of them). Of course,
all
> > > > > compilers might not support it, if so we'd need some way to abstract
> this
> > as
> > > > > reed suggested, though I have no idea how to do so.
> > > > 
> > > > How would that look?  Don't you still have the problem of the
preprocessor
> > > being
> > > > a single pass?
> > > > 
> > > > #define FOO(...) #define ...
> > > 
> > > Ah, I meant to avoid the "which one do I protect" issue. Like
> > > 
> > > #define SkAutoDisableDirectionCheck(...)
> > > 
> > > I have no idea how one might trick the identifier in "#define
<identifier>"
> > into
> > > being a computed thing.
> > 
> > Ah!  This is a good idea.  Variadic macros are pretty nearly universally
> > supported.  I'll follow this up with a patch to switch to (...), separate
from
> > this in case I have to roll it back.
> > 
> > Before I figured out it wasn't possible, I was feeling clever and thought,
"Oh
> > maybe we can just pull the #define outside the expression," like this,
> >   #define GUARDED_AUTO(classname) classname(...) SK_COMPILE_ASSERT(false,
> > missing_name_for_##classname)
> > then use it like
> >   #define GUARDED_AUTO(SkAutoFoo)
> > but, of course, that just fails while attempting to redefine GUARDED_AUTO. 
:(
> 
> Yeah, I was just looking at the C spec, and there isn't any macro expansion on
> the <identifier>. On the other hand, I think this would be clearer (at least
> show some intent / documentation) by being like
> 
> in SkTypes.h
> 
> // SkAutoFoo values should never be created without a name.
> #define SK_DISABLE_RVALUE_CONSTRUCTOR(classname) SK_COMPILE_ASSERT(false,
> rvalue_constuction_disabled_for_##classname)
> 
> after declaration
> 
> #define SkAutoDisableDirectionCheck(...)
> SK_DISABLE_RVALUE_CONSTRUCTOR(SkAutoDisableDirectionCheck)
> 
> So at least we have something a bit more explanatory at the declaration site.
At
> the moment you just have to know what this is there for. I had a bit of
trouble
> remembering why we were doing this when I first looked at the CL, and I was
> there when it was being discussed. This is still rather repeating itself, but
> due to no macro expansion available here, this might be the best one can do
with
> what we have.

Tried it out and I'm now totally on board.  I've used ... and added
SK_REQUIRE_NAME (rvalue/lvalue always seem so abstruse).  PTAL.

[caryclark, 2013-11-15 17:23:37]

lgtm

[caryclark, 2013-11-15 17:23:39]

lgtm

[reed1, 2013-11-15 17:41:39]

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

https://codereview.chromium.org/72603005/diff/110002/include/core/SkTypes.h#n...
include/core/SkTypes.h:152: // It's almost always a bug to create an SkAutoFoo
without a name.  This helps prevent that.
Like the feature, but I think the name is to generic. Also, can we write a
bigger dox?

e.g.
/**
 * For some classes, it is almost always an error to instantiate one w/o a
variable. e.g.
 *    SkAutoFree(ptr);
 *
 * In this case, the write meant to instantiate a local variable, so that the
destructor would
 * get called when this variable went out of scope. The problem with the above
line is that
 * it goes out of scope IMMEDIATELY.
 *
 *   SkAutoFree localvar(ptr);
 *
 * The above is correct usage. To ensure that callers don't forget this local
variable, definte
 * your_class(...) to be SK_REQUIRE_LOCAL_VAR(your_class). This will generate a
compile error
 * if someone tries to instantiate it w/o the variable.
 */
#define SK_REQUIRE_LOCAL_VAR(classname) ...

Not married to that name, but REQUIRE_NAME doesn't convey enough for me (am I
the only one?)

[mtklein, 2013-11-15 17:44:16]

On 2013/11/15 17:41:39, reed1 wrote:
> https://codereview.chromium.org/72603005/diff/110002/include/core/SkTypes.h
> File include/core/SkTypes.h (right):
> 
>
https://codereview.chromium.org/72603005/diff/110002/include/core/SkTypes.h#n...
> include/core/SkTypes.h:152: // It's almost always a bug to create an SkAutoFoo
> without a name.  This helps prevent that.
> Like the feature, but I think the name is to generic. Also, can we write a
> bigger dox?
> 
> e.g.
> /**
>  * For some classes, it is almost always an error to instantiate one w/o a
> variable. e.g.
>  *    SkAutoFree(ptr);
>  *
>  * In this case, the write meant to instantiate a local variable, so that the
> destructor would
>  * get called when this variable went out of scope. The problem with the above
> line is that
>  * it goes out of scope IMMEDIATELY.
>  *
>  *   SkAutoFree localvar(ptr);
>  *
>  * The above is correct usage. To ensure that callers don't forget this local
> variable, definte
>  * your_class(...) to be SK_REQUIRE_LOCAL_VAR(your_class). This will generate
a
> compile error
>  * if someone tries to instantiate it w/o the variable.
>  */
> #define SK_REQUIRE_LOCAL_VAR(classname) ...
> 
> Not married to that name, but REQUIRE_NAME doesn't convey enough for me (am I
> the only one?)

REQUIRE_NAMED_INSTANCES?

[mtklein, 2013-11-15 17:58:44]

updated

[reed1, 2013-11-15 18:20:11]

https://codereview.chromium.org/72603005/diff/100002/include/core/SkTSearch.h
File include/core/SkTSearch.h (right):

https://codereview.chromium.org/72603005/diff/100002/include/core/SkTSearch.h...
include/core/SkTSearch.h:129: SkAutoAsciiToLC(const char str[], size_t len =
(size_t)-1) {
Not sure I agree with this one.

1. This class does *not* perform its magic in its destructor as the AutoFoo guys
do, but it is used to access its lc() method. Seems less likely a caller would
make this mistake, and certainly it would not lead to any bug, unlike the Auto
guys.

2. The macro seems to force us to inline the code, which is overkill/bloaty for
this guy imho.

https://codereview.chromium.org/72603005/diff/100002/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/72603005/diff/100002/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:68: ~SkAutoBlitterChoose() {
Not sure its strictly required for this guy either, since its destructor's
side-effect is not the "point" of this class, but I care less, since its
destructor was already inlined and small.

[mtklein, 2013-11-15 18:28:13]

Yep, you caught me looking for SkAuto*.

https://codereview.chromium.org/72603005/diff/100002/include/core/SkTSearch.h
File include/core/SkTSearch.h (right):

https://codereview.chromium.org/72603005/diff/100002/include/core/SkTSearch.h...
include/core/SkTSearch.h:129: SkAutoAsciiToLC(const char str[], size_t len =
(size_t)-1) {
On 2013/11/15 18:20:12, reed1 wrote:
> Not sure I agree with this one.
> 
> 1. This class does *not* perform its magic in its destructor as the AutoFoo
guys
> do, but it is used to access its lc() method. Seems less likely a caller would
> make this mistake, and certainly it would not lead to any bug, unlike the Auto
> guys.
> 
> 2. The macro seems to force us to inline the code, which is overkill/bloaty
for
> this guy imho.

Reverted.

[reed1, 2013-11-15 18:40:31]

lgtm

[commit-bot: I haz the power, 2013-11-18 15:52:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/72603005/300001

[commit-bot: I haz the power, 2013-11-18 16:04:09]

Change committed as 12301