You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@thrift.apache.org by Stephen Haberman <st...@exigencecorp.com> on 2010/08/23 22:36:26 UTC
const bug
Hi,
We're running an old pre-0.3 thrift and trying upgrade, but running into
a compile error. I've reduce it down to this example:
struct A {
1: string name = ""
}
const A DEFAULT_A = {}
struct B {
1: A a = DEFAULT_A
}
The problem is that with "a = DEFAULT_A", we get this exception:
type error: const "a" was declared as struct/xception
If I remove "= DEFAULT_A", then it works great.
Just looking at the thrift file, using the DEFAULT_A const as a default
value seems to make sense. Is this a bug, or is it really illegal to use
const structs as default values?
Thanks,
Stephen
RE: const bug
Posted by Mark Slee <ms...@facebook.com>.
I agree it should be a copy. Very nasty bugs will likely creep in otherwise, when someone mutates one field of const-declared object instance (which most languages don't cleanly support making immutable).
In C++, it has to be a copy, since the fields are statically declared without references or pointers.
-----Original Message-----
From: Bryan Duxbury [mailto:bryan@rapleaf.com]
Sent: Monday, August 23, 2010 2:31 PM
To: thrift-user@incubator.apache.org
Subject: Re: const bug
Generally I would be leaning towards the copy approach.
On Mon, Aug 23, 2010 at 2:27 PM, Stephen Haberman
<st...@exigencecorp.com>wrote:
>
> > I think this one falls under the category of "reasonable, but
> > currently unimplemented feature."
>
> Upon closer examination, we had been using Thrift version
> 20080411-exported and the compiler accepted the previous input, but
> actually generated the wrong result.
>
> Given the previous thrift definitions, it was accepted, but B ended up
> having:
>
> public B() {
> this.a = new A();
> }
>
> When we assumed it had something like:
>
> public B() {
> this.a = Constants.DEFAULT_A;
> }
>
> We were lucky in that when we defined DEFAULT_A, we did not change
> any of its defaults, as in my example, so it was defined as:
>
> public class Constants {
> public static final A DEFAULT_A = new A();
> }
>
> Meaning we got the behavior we desired, by mere chance.
>
> It is nice that the compiler is stricter now, in that we're not allowed
> to use a feature that does not exist. :-)
>
> Thinking a bit more about it, having const structs as a default value
> poses the question: should the const be copied or just referenced? If it
> is just reference, mutating the const struct instance would affect any
> previously-constructed structs that were using it as a default value.
> Which could be a surprising behavior.
>
> I can open a ticket for this feature--any thoughts on whether the
> default value being a const struct should result in same reference or
> new copy semantics?
>
> Thanks,
> Stephen
>
>
Re: const bug
Posted by Bryan Duxbury <br...@rapleaf.com>.
Generally I would be leaning towards the copy approach.
On Mon, Aug 23, 2010 at 2:27 PM, Stephen Haberman
<st...@exigencecorp.com>wrote:
>
> > I think this one falls under the category of "reasonable, but
> > currently unimplemented feature."
>
> Upon closer examination, we had been using Thrift version
> 20080411-exported and the compiler accepted the previous input, but
> actually generated the wrong result.
>
> Given the previous thrift definitions, it was accepted, but B ended up
> having:
>
> public B() {
> this.a = new A();
> }
>
> When we assumed it had something like:
>
> public B() {
> this.a = Constants.DEFAULT_A;
> }
>
> We were lucky in that when we defined DEFAULT_A, we did not change
> any of its defaults, as in my example, so it was defined as:
>
> public class Constants {
> public static final A DEFAULT_A = new A();
> }
>
> Meaning we got the behavior we desired, by mere chance.
>
> It is nice that the compiler is stricter now, in that we're not allowed
> to use a feature that does not exist. :-)
>
> Thinking a bit more about it, having const structs as a default value
> poses the question: should the const be copied or just referenced? If it
> is just reference, mutating the const struct instance would affect any
> previously-constructed structs that were using it as a default value.
> Which could be a surprising behavior.
>
> I can open a ticket for this feature--any thoughts on whether the
> default value being a const struct should result in same reference or
> new copy semantics?
>
> Thanks,
> Stephen
>
>
Re: const bug
Posted by Stephen Haberman <st...@exigencecorp.com>.
> I think this one falls under the category of "reasonable, but
> currently unimplemented feature."
Upon closer examination, we had been using Thrift version
20080411-exported and the compiler accepted the previous input, but
actually generated the wrong result.
Given the previous thrift definitions, it was accepted, but B ended up
having:
public B() {
this.a = new A();
}
When we assumed it had something like:
public B() {
this.a = Constants.DEFAULT_A;
}
We were lucky in that when we defined DEFAULT_A, we did not change
any of its defaults, as in my example, so it was defined as:
public class Constants {
public static final A DEFAULT_A = new A();
}
Meaning we got the behavior we desired, by mere chance.
It is nice that the compiler is stricter now, in that we're not allowed
to use a feature that does not exist. :-)
Thinking a bit more about it, having const structs as a default value
poses the question: should the const be copied or just referenced? If it
is just reference, mutating the const struct instance would affect any
previously-constructed structs that were using it as a default value.
Which could be a surprising behavior.
I can open a ticket for this feature--any thoughts on whether the
default value being a const struct should result in same reference or
new copy semantics?
Thanks,
Stephen
Re: const bug
Posted by Stephen Haberman <st...@exigencecorp.com>.
> Agree, I'd love to see a ticket for this - and I'd love if it had a patch
> attached even more.
https://issues.apache.org/jira/browse/THRIFT-864
Sorry, no patch. :-)
- Stephen
Re: const bug
Posted by Bryan Duxbury <br...@rapleaf.com>.
Agree, I'd love to see a ticket for this - and I'd love if it had a patch
attached even more.
On Mon, Aug 23, 2010 at 1:45 PM, Mark Slee <ms...@facebook.com> wrote:
> I think this one falls under the category of "reasonable, but currently
> unimplemented feature." Not sure if there's a JIRA ticket open for this
> (nothing springs to mind). Worth opening one, I don't see any reason not to
> support this.
>
> -----Original Message-----
> From: Stephen Haberman [mailto:stephen@exigencecorp.com]
> Sent: Monday, August 23, 2010 1:36 PM
> To: thrift-user@incubator.apache.org
> Subject: const bug
>
> Hi,
>
> We're running an old pre-0.3 thrift and trying upgrade, but running into
> a compile error. I've reduce it down to this example:
>
> struct A {
> 1: string name = ""
> }
> const A DEFAULT_A = {}
> struct B {
> 1: A a = DEFAULT_A
> }
>
> The problem is that with "a = DEFAULT_A", we get this exception:
>
> type error: const "a" was declared as struct/xception
>
> If I remove "= DEFAULT_A", then it works great.
>
> Just looking at the thrift file, using the DEFAULT_A const as a default
> value seems to make sense. Is this a bug, or is it really illegal to use
> const structs as default values?
>
> Thanks,
> Stephen
>
>
RE: const bug
Posted by Mark Slee <ms...@facebook.com>.
I think this one falls under the category of "reasonable, but currently unimplemented feature." Not sure if there's a JIRA ticket open for this (nothing springs to mind). Worth opening one, I don't see any reason not to support this.
-----Original Message-----
From: Stephen Haberman [mailto:stephen@exigencecorp.com]
Sent: Monday, August 23, 2010 1:36 PM
To: thrift-user@incubator.apache.org
Subject: const bug
Hi,
We're running an old pre-0.3 thrift and trying upgrade, but running into
a compile error. I've reduce it down to this example:
struct A {
1: string name = ""
}
const A DEFAULT_A = {}
struct B {
1: A a = DEFAULT_A
}
The problem is that with "a = DEFAULT_A", we get this exception:
type error: const "a" was declared as struct/xception
If I remove "= DEFAULT_A", then it works great.
Just looking at the thrift file, using the DEFAULT_A const as a default
value seems to make sense. Is this a bug, or is it really illegal to use
const structs as default values?
Thanks,
Stephen