code review 1636044: Allow draw image destination to avoid slow path by impl...

Allow draw image destination to avoid slow path by implementing
DrawMask method.

[rog, 2010-06-11 13:07:23]

Hello nigeltao_golang, r (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r2, 2010-06-11 19:38:46]

i wonder about micromanaging this kind of stuff but this seems plausible to me

http://codereview.appspot.com/1636044/diff/2001/3001
File src/pkg/exp/draw/draw.go (left):

http://codereview.appspot.com/1636044/diff/2001/3001#oldcode106
src/pkg/exp/draw/draw.go:106: 
restore this blank line?

[rog, 2010-06-11 22:54:31]

i don't know if this is considered a good thing, but this
patch also enables "smart" image destinations, where
the image massages the draw operator in some way
before re-calling Draw on some underlying image,
for example by clipping or drawing to a different
location.

On 11 June 2010 20:38,  <r@google.com> wrote:
> i wonder about micromanaging this kind of stuff but this seems plausible
> to me
>
>
> http://codereview.appspot.com/1636044/diff/2001/3001
> File src/pkg/exp/draw/draw.go (left):
>
> http://codereview.appspot.com/1636044/diff/2001/3001#oldcode106
> src/pkg/exp/draw/draw.go:106:
> restore this blank line?
>
> http://codereview.appspot.com/1636044/show
>

[nigeltao, 2010-06-16 04:22:06]

http://codereview.appspot.com/1636044/diff/2001/3001
File src/pkg/exp/draw/draw.go (right):

http://codereview.appspot.com/1636044/diff/2001/3001#newcode74
src/pkg/exp/draw/draw.go:74: switch dst0 := dst.(type) {
This won't work if dst is a MyImage type and MyImage implements DrawMasker,
right? You need to pass in the MyImage explicitly cast as a DrawMasker. I think
that that's an API subtlety that will confuse people.

Instead of making a type switch (i.e. dst.(type)), you should make a type
assertion dst0, ok := dst.(DrawMasker). But I'm not sure if the type assertion
belongs inside the DrawMask function or whether that's the responsibility of the
caller of DrawMask. If a DrawMasker's implementation falls back on calling
DrawMask, then we could easily infinite loop.

Yet another thing to think about...

[rog, 2010-06-16 07:28:21]

On 16 June 2010 05:22,  <nigeltao@golang.org> wrote:
>
> http://codereview.appspot.com/1636044/diff/2001/3001
> File src/pkg/exp/draw/draw.go (right):
>
> http://codereview.appspot.com/1636044/diff/2001/3001#newcode74
> src/pkg/exp/draw/draw.go:74: switch dst0 := dst.(type) {
> This won't work if dst is a MyImage type and MyImage implements
> DrawMasker, right? You need to pass in the MyImage explicitly cast as a
> DrawMasker. I think that that's an API subtlety that will confuse
> people.

no, this will work fine in that case. type switches work just like
type assertions for interface types.

that was the whole point of the change.

> Instead of making a type switch (i.e. dst.(type)), you should make a
> type assertion dst0, ok := dst.(DrawMasker). But I'm not sure if the
> type assertion belongs inside the DrawMask function or whether that's
> the responsibility of the caller of DrawMask. If a DrawMasker's
> implementation falls back on calling DrawMask, then we could easily
> infinite loop.

that scenario is just like calling x.String() from within x.String() - you
need to change the static type.

[nigeltao, 2010-06-16 23:04:21]

On 2010/06/16 07:28:21, rog wrote:
> no, this will work fine in that case. type switches work just like
> type assertions for interface types.

Oops, you're right.

Still thinking...