You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by davinchia <gi...@git.apache.org> on 2017/07/20 20:25:56 UTC
[GitHub] thrift pull request #1311: Fix for constant assignments to optional fields i...
GitHub user davinchia opened a pull request:
https://github.com/apache/thrift/pull/1311
Fix for constant assignments to optional fields in Go.
As per ticket https://issues.apache.org/jira/browse/THRIFT-4253.
As I dug through the code I realised the problem was not only limited to optional strings, and that the generator was not generating optionals for all types.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/davinchia/thrift constant-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1311.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 #1311
----
commit de3d3a76bdaf2baa3eab9c6fdd8d3b31e7887f55
Author: Davin Chia Ming Juay <dc...@liveramp.com>
Date: 2017-07-20T08:08:48Z
logging to track path; modified function to pass optional value over
commit f038cc0d6504ecbf30bd13a1daa0f8fc8201fad4
Author: Davin Chia Ming Juay <dc...@liveramp.com>
Date: 2017-07-20T08:53:24Z
fix generation for bool and int constant values
commit 7c58a2555203aac67aec3d81c71dd807def4de59
Author: Davin Chia Ming Juay <dc...@liveramp.com>
Date: 2017-07-20T09:05:06Z
fix for double; make sure int type is precise
commit 04f07888c714415a94121a774d26738eda0b21ac
Author: Davin Chia Ming Juay <dc...@liveramp.com>
Date: 2017-07-20T09:13:30Z
fix for string
commit acd26f0e9e1c0fc47122b9f2a16573ee84fcc67e
Author: Davin Chia Ming Juay <dc...@liveramp.com>
Date: 2017-07-20T09:24:30Z
remove binary from opt because it is not needed
commit 54e97c3fc09737a15380d82d9cd5a9ac049d9095
Author: Davin Chia Ming Juay <dc...@liveramp.com>
Date: 2017-07-20T20:23:05Z
remove logging
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1311: Fix for constant assignments to optional fields in Go.
Posted by davinchia <gi...@git.apache.org>.
Github user davinchia commented on the issue:
https://github.com/apache/thrift/pull/1311
@Jens-G Github must have hidden my comments. My latest commit combined the functions and removed the toString function as per your suggestions.
Not sure why one of the integration tests are failing. Logs show `/usr/include/php5/Zend/zend_hash.h:266:2:` is the cause, which is not something I modified.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1311: Fix for constant assignments to optional fields in Go.
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1311
Ping
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1311: Fix for constant assignments to optional fields in Go.
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1311
@davinchia we've cleared out the CI build issues, so if you could squash your changes to one commit then rebase on master, then do a force push to refresh this pull request, it'll kick off a new build. Squashing will make it easier to apply the patch when the build completes successfully.
---
[GitHub] thrift pull request #1311: Fix for constant assignments to optional fields i...
Posted by davinchia <gi...@git.apache.org>.
Github user davinchia commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1311#discussion_r129124617
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -424,6 +424,52 @@ bool t_go_generator::is_pointer_field(t_field* tfield, bool in_container_value)
throw "INVALID TYPE IN type_to_go_type: " + type->get_name();
}
+string t_go_generator::gen_opt_const_values_(t_base_type::t_base tbase, t_const_value *value) {
+ string go_value_str = "&(&struct{x ";
+ switch (tbase) {
+ case t_base_type::TYPE_BOOL:
+ go_value_str += "bool}{";
+ go_value_str += (value->get_integer() > 0 ? "true" : "false");
+ break;
+
+ case t_base_type::TYPE_I8:
+ go_value_str += "int8}{";
+ go_value_str += std::to_string(static_cast<long long>(value->get_integer()));
+ break;
--- End diff --
a) The casts are to avoid the `ambiguous calls` errors I was seeing on the integration tests.
b) c) I separated the methods as I had mini unit tests on my machine. I left it this way since I felt the main method was getting slightly too long. Combining both methods will remove the casting problem and I have no strong opinions so I will do that instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1311: Fix for constant assignments to optional fields in Go.
Posted by davinchia <gi...@git.apache.org>.
Github user davinchia commented on the issue:
https://github.com/apache/thrift/pull/1311
@Jens-G
This is the first time I'm writing production C++ code so please feel free to tear this apart.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1311: Fix for constant assignments to optional fields i...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1311#discussion_r128901829
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -424,6 +424,52 @@ bool t_go_generator::is_pointer_field(t_field* tfield, bool in_container_value)
throw "INVALID TYPE IN type_to_go_type: " + type->get_name();
}
+string t_go_generator::gen_opt_const_values_(t_base_type::t_base tbase, t_const_value *value) {
+ string go_value_str = "&(&struct{x ";
+ switch (tbase) {
+ case t_base_type::TYPE_BOOL:
+ go_value_str += "bool}{";
+ go_value_str += (value->get_integer() > 0 ? "true" : "false");
+ break;
+
+ case t_base_type::TYPE_I8:
+ go_value_str += "int8}{";
+ go_value_str += std::to_string(static_cast<long long>(value->get_integer()));
+ break;
--- End diff --
Three questions:
a) Why are these casts needed?
b) Can we move that whole method next to the other method?
c) Why do we need two separate methods at all?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1311: Fix for constant assignments to optional fields in Go.
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1311
@davinchia we've cleared out the CI build issues, so if you could squash your changes to one commit then rebase on master, then do a force push to refresh this pull request, it'll kick off a new build. Squashing will make it easier to apply the patch when the build completes successfully.
---
[GitHub] thrift issue #1311: Fix for constant assignments to optional fields in Go.
Posted by davinchia <gi...@git.apache.org>.
Github user davinchia commented on the issue:
https://github.com/apache/thrift/pull/1311
https://stackoverflow.com/questions/30716354/how-do-i-do-a-literal-int64-in-go is a good summary of the different ways to assign constant values in Go.
I decided to use an anonymous struct since it seems to be the most self contained and efficient way to do so.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1311: Fix for constant assignments to optional fields in Go.
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1311
@davinchia we've cleared out the CI build issues, so if you could squash your changes to one commit then rebase on master, then do a force push to refresh this pull request, it'll kick off a new build. Squashing will make it easier to apply the patch when the build completes successfully.
---