You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Bryan Duxbury (JIRA)" <ji...@apache.org> on 2008/08/20 04:18:44 UTC
[jira] Created: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Isset fields for non-primitive types unnecessary
------------------------------------------------
Key: THRIFT-116
URL: https://issues.apache.org/jira/browse/THRIFT-116
Project: Thrift
Issue Type: Improvement
Components: Compiler (Java)
Reporter: Bryan Duxbury
Priority: Minor
For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12674350#action_12674350 ]
David Reiss commented on THRIFT-116:
------------------------------------
LGTM.
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116-v2.patch, thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672481#action_12672481 ]
David Reiss commented on THRIFT-116:
------------------------------------
I see a lot of places in the generator where you do something like
{code}
if (type_can_be_null(ftype)) {
out << field_name << "!= null";
} else {
out << "__isset." << field_name;
}
{code}
Is there a reason you don't use isSetField more?
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-116:
---------------------------------
Patch Info: [Patch Available]
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-116:
---------------------------------
Attachment: thrift-116.patch
This patch removes isset fields for all object types, replacing them without outright null checks. I've fixed the few tests that were broken by this (assist by David), and everything seems to be working again.
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672491#action_12672491 ]
Bryan Duxbury commented on THRIFT-116:
--------------------------------------
My thinking was that using code generation would save us method call overhead in the structs, but I'm probably stupid trying to out-think the java compiler and vm.
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-116:
---------------------------------
Attachment: thrift-116-v2.patch
This version cleans up some of the duplicated code you mentioned, along with some consolidation and cleanup around other things dealing with isset and null fields, etc.
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116-v2.patch, thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury reassigned THRIFT-116:
------------------------------------
Assignee: Bryan Duxbury
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672485#action_12672485 ]
Bryan Duxbury commented on THRIFT-116:
--------------------------------------
I'll try to collapse that repeated code down into a helper method.
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-116:
---------------------------------
Issue Type: Sub-task (was: Improvement)
Parent: THRIFT-115
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Priority: Minor
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury resolved THRIFT-116.
----------------------------------
Resolution: Fixed
I just committed this.
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116-v2.patch, thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-116) Isset fields for non-primitive types
unnecessary
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672486#action_12672486 ]
David Reiss commented on THRIFT-116:
------------------------------------
There's already a helper method in the generated code (isSetFieldName). Why not use that?
> Isset fields for non-primitive types unnecessary
> ------------------------------------------------
>
> Key: THRIFT-116
> URL: https://issues.apache.org/jira/browse/THRIFT-116
> Project: Thrift
> Issue Type: Sub-task
> Components: Compiler (Java)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-116.patch
>
>
> For fields that aren't primitives, the Isset structure doesn't need to contain flags for those fields. This is because it is easy to detect whether it was set or not just by checking whether the field's value is null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.