You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Ben Taitelbaum (JIRA)" <ji...@apache.org> on 2009/07/21 17:01:14 UTC
[jira] Created: (THRIFT-544) multiple enums with the same key
generate invalid code
multiple enums with the same key generate invalid code
------------------------------------------------------
Key: THRIFT-544
URL: https://issues.apache.org/jira/browse/THRIFT-544
Project: Thrift
Issue Type: Bug
Components: Compiler (Erlang)
Affects Versions: 0.1
Reporter: Ben Taitelbaum
Fix For: 0.1
The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
{code:title=EnumTest.thrift|borderStyle=solid}
enum MyType1 {
A = 0,
B = 1
}
enum MyType2 {
A = 2,
C = 4
}
{code}
produces:
{code:title=enumTest_types.hrl|borderStyle=solid}
-ifndef(_enumTest_types_included).
-define(_enumTest_types_included, yeah).
-define(enumTest_A, 0).
-define(enumTest_B, 1).
-define(enumTest_A, 2).
-define(enumTest_C, 4).
-endif.
{code}
In the patched version, it produces this:
{code:title=enumTest_types.hrl|borderStyle=solid}
-ifndef(_enumTest_types_included).
-define(_enumTest_types_included, yeah).
-define(enumTest_MyType1_A, 0).
-define(enumTest_MyType1_B, 1).
-define(enumTest_MyType2_A, 2).
-define(enumTest_MyType2_C, 4).
-endif.
{code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12733749#action_12733749 ]
Todd Lipcon commented on THRIFT-544:
------------------------------------
I agree with David here - the breaking of backwards compatibility seems like too much to me to be worth it.
On the other hand, I might support a patch which changes enum generation in erlang to use atoms like 'A' and 'B' instead of -defines at all. It's more erlangy - the bindings use -defines here for historical reasons. Since I'm no longer an active user of the bindings, though, I'll defer to Eugene, Chris, and David (or anyone else who uses them and wants to pipe up) for whether they think that's a good idea.
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Erlang)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12740150#action_12740150 ]
Bryan Duxbury commented on THRIFT-544:
--------------------------------------
I didn't try it out, but its very simple and looks good to me. +1
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (General)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch, thrift-544_prevent_redefining_consts.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-544:
---------------------------------
Component/s: (was: Compiler (Erlang))
Compiler (General)
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (General)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch, thrift-544_prevent_redefining_consts.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury closed THRIFT-544.
--------------------------------
Assignee: Ben Taitelbaum
Resolution: Fixed
I made the change that David suggested and committed this.
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (General)
> Affects Versions: 0.2
> Reporter: Ben Taitelbaum
> Assignee: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.4
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch, thrift-544_prevent_redefining_consts.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Reiss updated THRIFT-544:
-------------------------------
Priority: Minor (was: Major)
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Erlang)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "Ben Taitelbaum (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ben Taitelbaum updated THRIFT-544:
----------------------------------
Attachment: thrift-544_prevent_redefining_consts.patch
Here's a really simple patch that prevents multiple constants with the same name.
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Erlang)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch, thrift-544_prevent_redefining_consts.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12908092#action_12908092 ]
David Reiss commented on THRIFT-544:
------------------------------------
This was committed as r982825. It doesn't show up in the "all" tab because it was committed with a tag of "THRIFT-554".
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (General)
> Affects Versions: 0.2
> Reporter: Ben Taitelbaum
> Assignee: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.4
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch, thrift-544_prevent_redefining_consts.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "Ben Taitelbaum (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ben Taitelbaum updated THRIFT-544:
----------------------------------
Attachment: thrift-544_add_enum_name_to_enum_keys.patch
This fixes the issue, although it does change the names defined, so it may break existing code, but I can't see any other way around this.
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Erlang)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12740160#action_12740160 ]
David Reiss commented on THRIFT-544:
------------------------------------
I think the patch is correct, but the argument for correctness is very complicated because constants_[name] is a mutating operation that creates an entry if the "name" key is not present. I'd much prefer the condition be "constants_.find(name) != constants.end()"
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (General)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Priority: Minor
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch, thrift-544_prevent_redefining_consts.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-544) multiple enums with the same key
generate invalid code
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12733734#action_12733734 ]
David Reiss commented on THRIFT-544:
------------------------------------
This is an error in C as well, which is what Thrift's enums are based on. So I think the real solution is to throw an error at code generation time.
> multiple enums with the same key generate invalid code
> ------------------------------------------------------
>
> Key: THRIFT-544
> URL: https://issues.apache.org/jira/browse/THRIFT-544
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Erlang)
> Affects Versions: 0.1
> Reporter: Ben Taitelbaum
> Fix For: 0.1
>
> Attachments: thrift-544_add_enum_name_to_enum_keys.patch
>
>
> The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).
> {code:title=EnumTest.thrift|borderStyle=solid}
> enum MyType1 {
> A = 0,
> B = 1
> }
> enum MyType2 {
> A = 2,
> C = 4
> }
> {code}
> produces:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_A, 0).
> -define(enumTest_B, 1).
> -define(enumTest_A, 2).
> -define(enumTest_C, 4).
> -endif.
> {code}
> In the patched version, it produces this:
> {code:title=enumTest_types.hrl|borderStyle=solid}
> -ifndef(_enumTest_types_included).
> -define(_enumTest_types_included, yeah).
> -define(enumTest_MyType1_A, 0).
> -define(enumTest_MyType1_B, 1).
> -define(enumTest_MyType2_A, 2).
> -define(enumTest_MyType2_C, 4).
> -endif.
> {code}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.