You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by viveksjain <gi...@git.apache.org> on 2016/09/07 17:39:04 UTC
[GitHub] thrift pull request #1083: THRIFT-3921 Add C++ ostream operator<< functions ...
GitHub user viveksjain opened a pull request:
https://github.com/apache/thrift/pull/1083
THRIFT-3921 Add C++ ostream operator<< functions for enums
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/viveksjain/thrift THRIFT-3921
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1083.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 #1083
----
commit 570717279501ab1c0184ab76b93b8b35abae2914
Author: Vivek Jain <vi...@vivekja.in>
Date: 2016-09-07T17:34:25Z
THRIFT-3921 Add C++ ostream operator<< functions for enums
----
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1083
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1083
@alexandruavadanii thanks, I reopened the ticket. Looks like it might be a C++11 issue?
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by alexandruavadanii <gi...@git.apache.org>.
Github user alexandruavadanii commented on the issue:
https://github.com/apache/thrift/pull/1083
Hi,
This seems to have caused a regression on AArch64. I didn't investigate it yet, usually it's some signed vs unsigned char in compiler defaults, maybe some other type mismatches.
Logs: http://paste.ubuntu.com/24050156/
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by viveksjain <gi...@git.apache.org>.
Github user viveksjain commented on the issue:
https://github.com/apache/thrift/pull/1083
Yeah, as I mentioned on JIRA there may be backwards compatibility concerns. But from what I saw in the related [THRIFT-3336 comment](https://issues.apache.org/jira/browse/THRIFT-3336?focusedCommentId=14904949&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14904949) my sense was devs don't want to add more options/annotations, which makes sense to me since ultimately it does have a maintenance cost.
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1083
I'm going to pick this up and implement it in the context of THRIFT-4060. The compiler will emit a declaration for operator << of an enumeration, but the emitter for the definition of same will be optional.
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1083
Hello - when I upgraded a project to thrift-0.10.0 I found I needed to provide the operator << for enumerations. I am a bit concerned we're not providing an operator >> as well; I think it would be a good idea to make sure the implementation can be overridden. That said, if you could rebase on master and force push it will kick a new CI build; I would like to see things passing (save maybe build job 17 which has had some trouble lately).
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions ...
Posted by viveksjain <gi...@git.apache.org>.
Github user viveksjain commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1083#discussion_r77875285
--- Diff: compiler/cpp/src/generate/t_cpp_generator.cc ---
@@ -541,6 +542,35 @@ void t_cpp_generator::generate_enum(t_enum* tenum) {
<< tenum->get_name() << "Values"
<< ", _k" << tenum->get_name() << "Names), "
<< "::apache::thrift::TEnumIterator(-1, NULL, NULL));" << endl << endl;
+
+ generate_enum_ostream_operator(tenum);
+}
+
+void t_cpp_generator::generate_enum_ostream_operator(t_enum* tenum) {
+ f_types_ << "inline std::ostream& operator<<(std::ostream& out, const ";
+ if (gen_pure_enums_) {
+ f_types_ << tenum->get_name();
+ } else {
+ f_types_ << tenum->get_name() << "::type";
--- End diff --
I haven't read anything particular about `operator<<`, but in general I think for classes you want a reference, and for numeric types (enums included) you want the value \u2013 pass by reference is probably actually slower.
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by alexandruavadanii <gi...@git.apache.org>.
Github user alexandruavadanii commented on the issue:
https://github.com/apache/thrift/pull/1083
To be honest, my C++ is a bit rusty, I got here indirectly while investigating a downstream bug (https://jira.opnfv.org/browse/ARMBAND-224).
Btw, OS/compiler info: Ubuntu Xenial (16.04) AArch64 - native build on armv8 baremetal;
$ g++ --version
g++ (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.2) 5.4.0 20160609
I'll try to look at the code/logs a little, see if I can be of help.
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1083#discussion_r77873190
--- Diff: compiler/cpp/src/generate/t_cpp_generator.cc ---
@@ -541,6 +542,35 @@ void t_cpp_generator::generate_enum(t_enum* tenum) {
<< tenum->get_name() << "Values"
<< ", _k" << tenum->get_name() << "Names), "
<< "::apache::thrift::TEnumIterator(-1, NULL, NULL));" << endl << endl;
+
+ generate_enum_ostream_operator(tenum);
+}
+
+void t_cpp_generator::generate_enum_ostream_operator(t_enum* tenum) {
+ f_types_ << "inline std::ostream& operator<<(std::ostream& out, const ";
+ if (gen_pure_enums_) {
+ f_types_ << tenum->get_name();
+ } else {
+ f_types_ << tenum->get_name() << "::type";
--- End diff --
I'm sure it's nothing important in this context, but the recommended signature for overloading operator << is to use a const ref for the value.
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1083
I can see cases where someone relying on the enumeration's numeric output today would have a breaking change that they cannot easily avoid here. Changes like this one, for example the one that added what() to thrift C++ generated exception code, have caused me headaches in the past when upgrading. This makes me wonder if we want to make things like this thrift-compiler-time optional. Not something we have to solve here, but a more general question to the maintainers.
---
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 #1083: THRIFT-3921 Add C++ ostream operator<< functions for enu...
Posted by viveksjain <gi...@git.apache.org>.
Github user viveksjain commented on the issue:
https://github.com/apache/thrift/pull/1083
Is there anything else needed to merge this? Seems to be a lot of stale PRs waiting for review/merge. :(
---
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.
---