[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16349640#comment-16349640
]
Craig Ringer commented on THRIFT-2471:
--
I think a mistake may have been made in this commit, because {{cpp.ref}} still
exists in the tree but only apparently for the Go compiler:
{code}
CHANGES:* [THRIFT-2471] - Make cpp.ref annotation language agnostic
compiler/cpp/src/thrift/generate/t_go_generator.cc: if
(tfield->annotations_.count("cpp.ref") != 0) {
lib/go/test/RefAnnotationFieldsTest.thrift: 1: optional string s = "DEFAULT"
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 2: optional i64 i = 42 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 3: optional bool b = false (cpp.ref
= ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 4: optional string s2 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 5: optional i64 i2 (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 6: optional bool b2 (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 7: optional structA aa (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 9: optional list l (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 10: optional list l2 = [1, 2]
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 11: optional map m
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 12: optional map m2 =
{1:2, 3:4} (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 13: optional binary bin (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 14: optional binary bin2 = "asdf"
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 15: required string ref_s =
"DEFAULT" (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 16: required i64 ref_i = 42
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 17: required bool ref_b = false
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 18: required string ref_s2 (cpp.ref
= ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 19: required i64 ref_i2 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 20: required bool ref_b2 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 21: required structA ref_aa
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 22: required list ref_l
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 23: required list ref_l2 = [1,
2] (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 24: required map ref_m
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 25: required map ref_m2 =
{1:2, 3:4} (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 26: required binary ref_bin
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 27: required binary ref_bin2 =
"asdf" (cpp.ref = ""),
{code}
and a {{git grep '\'}} doesn't reveal anything that looks relevant; in
particular nothing at all in the c++ codegen.
Looks to me like support in Go was missed when converting this to the {{&}}
form.
There's also another oversight where it produces bad code for {{optional}}
members, but I'll raise a new bug for that.
> Make cpp.ref annotation language agnostic
> -
>
> Key: THRIFT-2471
> URL: https://issues.apache.org/jira/browse/THRIFT-2471
> Project: Thrift
> Issue Type: Improvement
> Components: Compiler (General)
>Reporter: Jens Geyer
>Assignee: Jens Geyer
>Priority: Major
> Fix For: 0.9.2
>
> Attachments: boost_1.40.0__to__1.54.0-patch
>
>
> The proposal is to make the new {{cpp.ref}} annotation introduced with
> THRIFT-2421 language agnostic instead of a C++ specialty only.
> The annotation changes inlined nested structs into pointers to structs. This
> behaviour is potentially relevant with all languages using value semantics
> for nested structs etc.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15873176#comment-15873176
]
ASF GitHub Bot commented on THRIFT-2471:
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/113
Sorry, #1195 has nothing to do with this one. I was off by one.
> Make cpp.ref annotation language agnostic
> -
>
> Key: THRIFT-2471
> URL: https://issues.apache.org/jira/browse/THRIFT-2471
> Project: Thrift
> Issue Type: Improvement
> Components: Compiler (General)
>Reporter: Jens Geyer
>Assignee: Jens Geyer
> Fix For: 0.9.2
>
> Attachments: boost_1.40.0__to__1.54.0-patch
>
>
> The proposal is to make the new {{cpp.ref}} annotation introduced with
> THRIFT-2421 language agnostic instead of a C++ specialty only.
> The annotation changes inlined nested structs into pointers to structs. This
> behaviour is potentially relevant with all languages using value semantics
> for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14026352#comment-14026352
]
Henrique Mendonça commented on THRIFT-2471:
---
Hi Jens,
It seems to require libboost-dev 1.54 on Ubuntu. I've changed it so it'd run on
travis too. (Committing next)
I think we still need to install it on Jenkins or it simply ignores the cpp
library.
By the way, TFDTransportTest fails now! Does anyone has any idea why? There
have been no changes in theses files since more than a year, so it must come
from recent patches elsewhere.
please see: https://travis-ci.org/henrique/thrift/builds/27206877#L2602
Best,
Henrique
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Assignee: Jens Geyer
Fix For: 0.9.2
Attachments: boost_1.40.0__to__1.54.0-patch
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14026467#comment-14026467
]
Hudson commented on THRIFT-2471:
SUCCESS: Integrated in Thrift #1175 (See
[https://builds.apache.org/job/Thrift/1175/])
THRIFT-2471 requires libboost 1.54 (henrique: rev
48b189716f7611a73e9a2d9b4e2f3989c101740f)
* configure.ac
* doc/install/README.md
* .travis.yml
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Assignee: Jens Geyer
Fix For: 0.9.2
Attachments: boost_1.40.0__to__1.54.0-patch
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13989108#comment-13989108
]
Jens Geyer commented on THRIFT-2471:
boost 1_53_0, everything fine over here.
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
Attachments: boost_1.40.0__to__1.54.0-patch
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13989137#comment-13989137
]
Hudson commented on THRIFT-2471:
FAILURE: Integrated in Thrift #1146 (See
[https://builds.apache.org/job/Thrift/1146/])
THRIFT-2471 Make cpp.ref annotation language agnostic (jensg: rev
885c6791235e011794a0b65c00f1d9fdf3d233e3)
* compiler/cpp/src/thriftl.ll
* lib/cpp/src/thrift/protocol/TProtocol.h
* compiler/cpp/src/generate/t_cpp_generator.cc
* compiler/cpp/src/parse/t_field.h
* lib/cpp/src/thrift/protocol/TProtocolException.h
* compiler/cpp/src/thrifty.yy
* test/Recursive.thrift
* lib/cpp/test/RecursiveTest.cpp
THRIFT-2471: Make cpp.ref annotation language agnostic (roger: rev
1953e21a373c2bfd9e8a0804b71297039319021c)
* configure.ac
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
Attachments: boost_1.40.0__to__1.54.0-patch
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13988119#comment-13988119
]
Roger Meier commented on THRIFT-2471:
-
+1
do you commit Jens?
@all: doc updates are welcome ;-)
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13988185#comment-13988185
]
ASF GitHub Bot commented on THRIFT-2471:
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/113
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13986115#comment-13986115
]
ASF GitHub Bot commented on THRIFT-2471:
GitHub user djwatson opened a pull request:
https://github.com/apache/thrift/pull/113
THRIFT-2471
three changes as discussed in THRIFT-2471:
* Update parser to use instead of cpp.ref
* Recursion depth limit, a la protobufs
* shared_ptr instead of raw pointer w/deletes in the destructor.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/djwatson/thrift cpp.ref
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/113.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #113
commit 52b99af4ee1574253dcb77933d76a7ebb2d830df
Author: Dave Watson davejwat...@fb.com
Date: 2014-04-23T20:05:56Z
change cpp.ref to
commit 3f9d31cc6140367529fd8f7b1b67056ec321786f
Author: Dave Watson davejwat...@fb.com
Date: 2014-04-23T21:50:29Z
Recursion depth limit
commit 61468e4534ce9e6a4f4f643bfd00542d13600d83
Author: Dave Watson davejwat...@fb.com
Date: 2014-04-25T19:59:18Z
shared_ptr for reference type
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13978567#comment-13978567
]
Ben Craig commented on THRIFT-2471:
---
[~henrique] : There are some challenges with using non-owning pointers. On the
client side, things are easy enough, but on the server side, the processor
would need to own the referenced objects and clean them up. This is doable,
but it is unusual in comparison to the rest of the serialization.
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13978613#comment-13978613
]
Henrique Mendonça commented on THRIFT-2471:
---
Hi guys,
Yes Ben, I see the problematic. I think this annotation only creates the
illusion that some real references would be possible. We could restore the
original implementation with a scoped_ptr as Randy suggested but IMO it won't
change much.
I'd prefer removing it, or really supporting references...
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13977611#comment-13977611
]
Randy Abernethy commented on THRIFT-2471:
-
I like the shared_ptr approach because:
- it is consistent with the rest of the CPP Thrift API (shared_ptrprotocol,
etc.)
- it makes delete a non issue (object hangs around until thrift _and_ user are
finished with it)
- enables objects to appear multiple times in a DAG safely (though they will
be serialized each time)
- Allows the user to send the same objects multiple times in multiple RPC calls
(no new/delete/copy overhead, just ref up/down)
- Works with async interfaces (we need to have a say in the lifespan of the
objects if serialization happens out of band, I suspect people needing this
feature are likely to try passing big stuff which will make async that much
more important)
- Clearly implies cycles will not be a good thing (I think most know about
shared_ptr loops)
+1 for starting with DAGs only, offering no de-duplication, and detecting
cycles in the serializer (and throwing if found)
[~henrique] I agree, weak pointers could be a nice way to do cycles at the type
level, could then be serialized with the graph without touching anything at
the Protocol level.
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13975996#comment-13975996
]
Henrique Mendonça commented on THRIFT-2471:
---
+1 for changing the cpp.ref to simply * (or perhaps )
However, I don't think you should take ownership of the pointers, this is
confusing and unclear. I didn't realise it from the original patch, but I
strongly believe we need to change this before release. The user should handle
the pointer himself or use plain objects if he/she wants automatic memory
management. I don't see the point of having references otherwise as no
associations are allowed. In this case the reference symbol will probably
be more clear, and the implementation could also follow. Thoughts?
[~codesf] I always wished for a shared_ptr implementation but I think we'd need
to change everything to shared_ptr and the *cpp.ref to weak_ptr. It would
work on both boost and c++11. In this case _isset wouldn't be necessary any
longer ;)
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13976264#comment-13976264
]
Jens Geyer commented on THRIFT-2471:
Not bad. The {{}} proposal sounds good to me and would integrate the feature
more into the IDL.
@[~davejwatson]: Should we keep the existing cpp.ref annotation for FB
compatibility? What would you recommend?
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
Fix For: 0.9.2
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13973885#comment-13973885
]
Randy Abernethy commented on THRIFT-2471:
-
Hey Dave,
In the C++ implementation of cpp.ref did you consider shared_ptr? Not sure but
I think it might add some value over raw ptrs. The IDL below crashes with a
double delete if you ref the same object twice in the tree.
{code:title=tree.idl|borderStyle=solid}
struct tree {
1: tree left (cpp.ref=)
2: tree right (cpp.ref=)
}
service simple {
void hello(1: string msg, 2: tree t)
}
{code}
So this works:
{code:title=works.cpp|borderStyle=solid}
tree root;
root.left = new tree();
root.right = new tree();
client.hello(Hi, root);
{code}
and this doesn't:
{code:title=works.cpp|borderStyle=solid}
tree root;
tree *leg = new tree();
root.left = leg;
root.right = leg;
client.hello(Hi, root);
{code}
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13974267#comment-13974267
]
Dave Watson commented on THRIFT-2471:
-
Facebook's current use cases do not contain any cycles, nor repeated nodes -
internally our version uses unique_ptr, so the above wouldn't crash, but also
wouldn't work, since you couldn't move the same unique_ptr twice.
In the current implementation there are a lot of sharp edges around cycles /
one node referenced twice - see also THRIFT-2005, THRIFT-2462. We should
probably come up with a reasonable strategy to fix all of these. Shared_ptr
would be an easy fix in this case, but you'd still end up serializing the leg
twice
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13974274#comment-13974274
]
Ben Craig commented on THRIFT-2471:
---
I'd rather not try to fix cycles magically. MSRPC / DCE tries to do lots of
smart things with pointers, and it's hard to understand, and hard to maintain.
I could see adding cycle detection, and if we encounter a cycle, throw an
exception.
I'd rather us not tackle the issue of de-duplicating nodes in a graph and then
hooking the graph back together on the other end of the wire. That seems like
a reasonable thing to do on top of Thrift, but I don't want it done within
Thrift.
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13974313#comment-13974313
]
Randy Abernethy commented on THRIFT-2471:
-
unique_ptr is a great implementation. In its C++98 world I think Thrift uses
boost::share_ptr in several places where std::unique_ptr is the right answer.
I do think that sending multiple copies of repeated nodes in a DAG is a
reasonable thing for Thrift to do, which as Dave mentions is what shared_ptr
would give us. Another problem this fixes is the case where the user is using
the object yet wants to serialize it by adding an additional reference to it in
the interface structure. The ref count goes up, thrift serializes it, the ref
count goes down. Quite a bit less expensive than copy construct/destroy. Given
that the Thrift interface is rife with shared_ptr I would expect developers to
understand this implementation fairly quickly. It would also mirror the
behavior of the dynamic programming languages.
I agree with Ben that we should at least detect cycles and throw rather than
crashing the stack (!). In explicable errors are not a hallmark of a great
framework, using shared_ptr to eliminate double deletes would also aid us here.
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13974315#comment-13974315
]
Jens Geyer commented on THRIFT-2471:
{quote}
I'd rather not try to fix cycles magically. MSRPC / DCE tries to do lots of
smart things with pointers, and it's hard to understand, and hard to maintain.
{quote}
That was one of the reasons for {{graph}} (see THRIFT-2005), it is designed
as an _extension_ on top of the the existing stuff, not interfering with it.
Internally, {{graph}} uses a container and serializes as a {{list}} of node
data. If one choses to use a {{graph}}, it would be an design decision.
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13972006#comment-13972006
]
Randy Abernethy commented on THRIFT-2471:
-
Hey All,
Just curious, is cpp.ref (and its proposed cross language support) necessary or
convenience? In other words, could the compiler is_reference() function figure
out if a struct is self referential deterministically without the assistance of
an annotation? If so, I wonder if it wouldn't be better to do the work at the
compiler level and keep the IDL simple (no annotation needed).
If we are targeting support for arbitrary (not just mandatory) references and
if this is going to apply to all languages, I wonder if it should not be a
first class feature of the IDL (rather than an annotation).
-Randy
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13972071#comment-13972071
]
Dave Watson commented on THRIFT-2471:
-
It's necessary for languages that use value semantics - CPP being the primary
example. Not necessary for ones that use fully boxed types - php, py, etc.
Making it a general annotation sounds like a fine proposal - although it would
only be necessary for some languages and not all, I'd probably still argue for
just an annotation vs. a first class part of the IDL
It can't be inferred - if we have a struct A that contains a B, and B contains
an A, what do we do? Are they both references? Or just one? Which one?
Protobufs solves this by making everything a reference.
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13972321#comment-13972321
]
Randy Abernethy commented on THRIFT-2471:
-
Hey Jens Dave, thanks for the brain dump on #thrift.
To Dave's point, in the A B A scenario I can imagine a case where the
semantic of A B is composition, implying that A has a B value, and the
semantic of B A is association, implying B may refer to an A with a
reference. The interface designer would need to be explicit here, precisely
because the interface semantics are different.
Adding references to Thrift is a big step, I don't see it as a language-centric
addition. I suspect developers would be surprised if an interface says A
contains a B and in some languages you don't get a B. If the semantic is
composition we should make sure that at the Thrift level there is always a B
for every A, even in Python and other dynamic languages.
I found a bit of production MIDL using composition association that helped me
think this over. The Plan struct here has a TimeStamp (composition) and a Wave
reference (association). When you create a Plan you get a TimeStamp but not a
Wave (regardless of your implementation language). You can attach a Wave,
detach a Wave, swap Waves but the TimeStamp is a fixed part of Plan.
{panel}
typedef struct _Plan
{
...
TimeStamp entryDateTime;
Wave* waveArray;
...
} Plan;
{panel}
Thrift was purely Composition based prior to the addition of cpp.ref. I think
cpp.ref made perfect sense in the context used and, as I understand it, the
approach was to only support DAGs, which simplifies things. As I read the tree
support code in THRIFT-2422, each object is serialized in place and if an
object appears twice in the graph it is serialized twice (essentially treated
as two separate copies of the same object). If your trees only contain unique
nodes this is simple and efficient. If your graph instances contain cycles
(e.g. THRIFT-2462) we will need to essentially do what graph (THRIFT-2005)
contemplates and serialize objects based on identity not based on the digraph.
So right now we have a way to countermand Thrift’s default composition
semantics but only in C++ and only with DAG support. I think Jens is correct to
seek a way to enable reference semantics universally. I also feel like
reference semantics are a pretty big deal and mark a material new addition to
Thrift, warranting a new serialization approach (if we plan to support instance
cycles).
My read on annotations is that they are for manipulating the implementation
independent of the interface. For example, making a type final/sealed in some
languages, or using a hash set instead of a BTree set in some language. These
things don’t affect the interface, only the implementation. What we contemplate
with references provides a big new interface feature as I see it, and it needs
to be the same semantic in every language. This motivates me to prefer an IDL
construct to an annotation. Just my 2 cents.
Best,
Randy
Make cpp.ref annotation language agnostic
-
Key: THRIFT-2471
URL: https://issues.apache.org/jira/browse/THRIFT-2471
Project: Thrift
Issue Type: Improvement
Components: Compiler (General)
Reporter: Jens Geyer
The proposal is to make the new {{cpp.ref}} annotation introduced with
THRIFT-2421 language agnostic instead of a C++ specialty only.
The annotation changes inlined nested structs into pointers to structs. This
behaviour is potentially relevant with all languages using value semantics
for nested structs etc.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
