thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Allen George (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (THRIFT-2945) Implement support for Rust language
Date Tue, 14 Feb 2017 02:20:42 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-2945?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864893#comment-15864893
] 

Allen George edited comment on THRIFT-2945 at 2/14/17 2:20 AM:
---------------------------------------------------------------

[~prz] - let me try to address these points individually.

# "Split the {{read_from_in_protocol}} and {{write_to_out_protocol}} as a trait." This seems
like a great idea. Perhaps it could be named {{TThriftType}} to keep with the existing naming
convention? I suggest submitting this as a PR since it would be small and self-contained.
# "Auto-derive hash on generated types." Are you sure this works with types that contain {{OrderedFloat}}?
Thrift doubles (represented in Rust by floats, not fixed-point integers) are the reason why
I had to use {{BTreeMap}} and {{BTreeSet}}, and was unable to auto-derive {{Hash}}. Perhaps
I overlooked something?
# Could you clarify why you need to change the enum and union code-generation? From what I
understand, your main requirement was getting a list of all defined enum variants and associated
values. Since we know the full list of variants ahead from the thrift file we could simply
add a {{values()}} method that returns the information you need. Also, I specifically opted
*not* to generate macros since it adds another level of indirection that anybody debugging
thrift auto-gen code would have to reason about: they'd have to understand how the thrift
gets turned into a macro, the macro definition, and then the final rust output.
# Finally, while I understand that you want to use the Thrift {{namespace}} directive, could
you give me a concrete example of what you expect the output to be? Would it be output to
different directories, or would you output it to the same directory and then rely on a manual
step afterwards to move it to the appropriate Rust module. (PS. Your patch has a bug: you
forgot to {{rust_safe_case}} the module name, and it will break on thrift modules that already
have underscores in them)

Another points: I looked at your branch, and noticed that your formatting is inconsistent
with the existing code (the existing code uses 2-space tabs).


was (Author: allengeorge):
[~prz] - let me try to address these points individually.

# "Split the {{read_from_in_protocol}} and {{write_to_out_protocol}} as a trait." This seems
like a great idea. Perhaps it could be named {{TThriftType}} to keep with the existing naming
convention? I suggest submitting this as a PR since it would be small and self-contained.
# "Auto-derive hash on generated types." Are you sure this works with types that contain {{OrderedFloat}}?
Thrift doubles (represented in Rust by floats, not fixed-point integers) are the reason why
I had to use {{BTreeMap}} and {{BTreeSet}}, and was unable to auto-derive {{Hash}}. Perhaps
I overlooked something?
# Could you clarify why you need to change the enum and union code-generation? From what I
understand, your main requirement was getting a list of all defined enum variants and associated
values. Since we know the full list of variants ahead from the thrift file we could simply
add a {{values()}} method that returns the information you need. Also, I specifically opted
*not* to generate macros since it adds another level of indirection that anybody debugging
thrift auto-gen code would have to reason about: they'd have to understand how the thrift
gets turned into a macro, the macro definition, and then the final rust output.
# Finally, while I understand that you want to use the Thrift {{namespace}} directive. Could
you give me a concrete example of what you expect the output to be? Would it be output to
different directories, or would you output it to the same directory and then rely on a manual
step afterwards to move it to the appropriate Rust module. (PS. Your patch has a bug: you
forgot to {{rust_safe_case}} the module name, and it will break on thrift modules that already
have underscores in them)

Another points: I looked at your branch, and noticed that your formatting is inconsistent
with the existing code (the existing code uses 2-space tabs).

> Implement support for Rust language
> -----------------------------------
>
>                 Key: THRIFT-2945
>                 URL: https://issues.apache.org/jira/browse/THRIFT-2945
>             Project: Thrift
>          Issue Type: New Feature
>          Components: Rust - Compiler, Rust - Library
>            Reporter: Maksim Golov
>            Assignee: Allen George
>             Fix For: 0.11.0
>
>
> Work on implementing support for Rust is in progress: https://github.com/maximg/thrift
by Simon GĂ©nier and myself.
> It will probably take quite some time to complete. Please keep us updated if there are
changes related to our work.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message