thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From chetmurthy <...@git.apache.org>
Subject [GitHub] thrift pull request #1435: THRIFT-4399 plugin.thrift t_const_value is not us...
Date Sat, 02 Dec 2017 02:03:41 GMT
GitHub user chetmurthy opened a pull request:

    https://github.com/apache/thrift/pull/1435

    THRIFT-4399 plugin.thrift t_const_value is not used as a union in C++…

    … code -- fix this
    
    plugin.thrift defines t_const_value as a union.  But in plugin_output.cc
    and plugin.cc, the converters clearly either (a) SET NEITHER of
    identifier_val & enum_val, or (b) SET BOTH.  But these are two different
    fields in the union.  So clearly, the type t_const_value isn't being
    treated as a union.
    
    I think we need to fix Thrift's treatment of unions, but independently,
    the plugin should use Thrift's type system in a correct manner.  This is
    easy-to-fix, but since the current plugin relies on a bug, the fix will
    be a breaking change.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/chetmurthy/thrift THRIFT-4399-plugin-t_const_value-union-fix

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/thrift/pull/1435.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 #1435
    
----
commit 3cf9b66f291779f34aba5355e5a5a75acfc029c0
Author: Chet Murthy <chetsky@gmail.com>
Date:   2017-12-02T02:01:28Z

    THRIFT-4399 plugin.thrift t_const_value is not used as a union in C++ code -- fix this
    
    plugin.thrift defines t_const_value as a union.  But in plugin_output.cc
    and plugin.cc, the converters clearly either (a) SET NEITHER of
    identifier_val & enum_val, or (b) SET BOTH.  But these are two different
    fields in the union.  So clearly, the type t_const_value isn't being
    treated as a union.
    
    I think we need to fix Thrift's treatment of unions, but independently,
    the plugin should use Thrift's type system in a correct manner.  This is
    easy-to-fix, but since the current plugin relies on a bug, the fix will
    be a breaking change.

----


---

Mime
View raw message