Fix for potential typedef issue

Fix for potential typedef issue

BUG=

[reed1, 2013-10-02 13:12:24]

Have we shown that we can get in trouble (i.e. fail silently) when we use
private? I just want to be sure *all* of these changes are necessary (to review
manually).

[sugoi1, 2013-10-02 14:38:51]

On 2013/10/02 13:12:24, reed1 wrote:
> Have we shown that we can get in trouble (i.e. fail silently) when we use
> private? I just want to be sure *all* of these changes are necessary (to
review
> manually).

Yes, you can silently fail. For example :

struct A {
    void print() { printf("A\n"); }
};

struct B : public A {
    void print() { printf("B\n"); }
    typedef A INHERITED;
};

struct C : public B {
    void print(INHERITED& type) {
        type.print();
    }
    typedef B INHERITED;
};

void test() {
    B b;
    C c;
    c.print(b);
}

This prints "A". When moving the "typedef B INHERITED;" on top of class C,
test() prints "B".

[sugoi1, 2013-10-02 14:43:38]

On 2013/10/02 14:38:51, sugoi1 wrote:
> On 2013/10/02 13:12:24, reed1 wrote:
> > Have we shown that we can get in trouble (i.e. fail silently) when we use
> > private? I just want to be sure *all* of these changes are necessary (to
> review
> > manually).
> 
> Yes, you can silently fail. For example :
> 
> struct A {
>     void print() { printf("A\n"); }
> };
> 
> struct B : public A {
>     void print() { printf("B\n"); }
>     typedef A INHERITED;
> };
> 
> struct C : public B {
>     void print(INHERITED& type) {
>         type.print();
>     }
>     typedef B INHERITED;
> };
> 
> void test() {
>     B b;
>     C c;
>     c.print(b);
> }
> 
> This prints "A". When moving the "typedef B INHERITED;" on top of class C,
> test() prints "B".

Oh, right, private, let me try again...

[mtklein, 2013-10-02 15:08:32]

On 2013/10/02 14:43:38, sugoi1 wrote:
> On 2013/10/02 14:38:51, sugoi1 wrote:
> > On 2013/10/02 13:12:24, reed1 wrote:
> > > Have we shown that we can get in trouble (i.e. fail silently) when we use
> > > private? I just want to be sure *all* of these changes are necessary (to
> > review
> > > manually).
> > 
> > Yes, you can silently fail. For example :
> > 
> > struct A {
> >     void print() { printf("A\n"); }
> > };
> > 
> > struct B : public A {
> >     void print() { printf("B\n"); }
> >     typedef A INHERITED;
> > };
> > 
> > struct C : public B {
> >     void print(INHERITED& type) {
> >         type.print();
> >     }
> >     typedef B INHERITED;
> > };
> > 
> > void test() {
> >     B b;
> >     C c;
> >     c.print(b);
> > }
> > 
> > This prints "A". When moving the "typedef B INHERITED;" on top of class C,
> > test() prints "B".
> 
> Oh, right, private, let me try again...

I was feeling thorough so I checked: all of the typedefs moved in this CL both
began as private and end up private.

[sugoi1, 2013-10-02 15:37:06]

Well, obviously, if class B had "friend class C;", it would fail silently. There
are some friend statements in skia, although it's not that common.
I can also imagine a scenario where INHERITED is a macro, like "#define
INHERITED A" above class C, which would also fail silently.
I'm sure I could invent all sorts of wacky scenarios for this, but I can't
foresee how future skia code may or may not fall into certain pitfalls that
would lead into this issue and different compilers may have different ways of
triggering this issue.
In the current context of SkFlattenable classes written as they are today, the
problem is not visible yet, but I'm not sure that I understand what is to be
lost by making trivial changes that makes this code more error proof.
Honestly, I don't want to spend too much time on this as this is yak shaving. At
this point, all I want is to get https://codereview.chromium.org/23021015
committed and the secure serialization in by M32. I was only making this
separate cl to make the reviewing process go faster, but I can ditch the changes
if it's actually going to make it go slower.

[reed1, 2013-10-02 16:13:38]

I'm sorry this is frustrating, and I certainly don't want to slow down the other
CL either.

I guess the point of my question was exactly you comment:
- this is a huge CL
- it is trivial (or is intended to be)
- it doesn't fix anything yet
- doesn't have any tests to know that we
    - don't regress in the future
    - didn't make something accidentally worse in this CL

I think we should consider skipping this change, since it seems (to me) that it
has more risk (e.g. accidental typo, takes your time to write, takes our time to
manually review) than the reward... and we don't have a way (yet) to confirm
that we don't "break" this in the future.

That said, if any of my assumptions are wrong, then my (mild) conclusion may
also be wrong.

[sugoi1, 2013-10-02 20:04:46]

On 2013/10/02 16:13:38, reed1 wrote:
> I'm sorry this is frustrating, and I certainly don't want to slow down the
other
> CL either.
> 
> I guess the point of my question was exactly you comment:
> - this is a huge CL
> - it is trivial (or is intended to be)
> - it doesn't fix anything yet
> - doesn't have any tests to know that we
>     - don't regress in the future
>     - didn't make something accidentally worse in this CL
> 
> I think we should consider skipping this change, since it seems (to me) that
it
> has more risk (e.g. accidental typo, takes your time to write, takes our time
to
> manually review) than the reward... and we don't have a way (yet) to confirm
> that we don't "break" this in the future.
> 
> That said, if any of my assumptions are wrong, then my (mild) conclusion may
> also be wrong.

I'm going to think about it a little bit more before I delete this issue, but,
in any case, let's put this CL on ice for now. The important part is what's in
the other CL and I removed all typedef "fixes" from it, so we can look at it
without the typedef distraction.

[reed1, 2013-10-03 08:42:13]

On 2013/10/02 20:04:46, sugoi1 wrote:
> On 2013/10/02 16:13:38, reed1 wrote:
> > I'm sorry this is frustrating, and I certainly don't want to slow down the
> other
> > CL either.
> > 
> > I guess the point of my question was exactly you comment:
> > - this is a huge CL
> > - it is trivial (or is intended to be)
> > - it doesn't fix anything yet
> > - doesn't have any tests to know that we
> >     - don't regress in the future
> >     - didn't make something accidentally worse in this CL
> > 
> > I think we should consider skipping this change, since it seems (to me) that
> it
> > has more risk (e.g. accidental typo, takes your time to write, takes our
time
> to
> > manually review) than the reward... and we don't have a way (yet) to confirm
> > that we don't "break" this in the future.
> > 
> > That said, if any of my assumptions are wrong, then my (mild) conclusion may
> > also be wrong.
> 
> I'm going to think about it a little bit more before I delete this issue, but,
> in any case, let's put this CL on ice for now. The important part is what's in
> the other CL and I removed all typedef "fixes" from it, so we can look at it
> without the typedef distraction.

SGTM -- I'm not pushing to delete it either, just wasn't ready to approve it