You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jeking3 <gi...@git.apache.org> on 2017/01/30 20:53:33 UTC
[GitHub] thrift pull request #1172: THRIFT-4060 add better support in the cpp generat...
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/1172
THRIFT-4060 add better support in the cpp generator for custom ostream operators on structures
This pull request adds support for an annotation that will disable the emit code for operator << and printTo, allowing a custom streaming output format to be provided by the consuming application.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift THRIFT-4060
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1172.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 #1172
----
commit db4cb328a374a75c91cf70c61a808b68a54f8699
Author: James E. King, III <ji...@simplivity.com>
Date: 2017-01-30T20:51:42Z
THRIFT-4060 add better support in the cpp generator for custom ostream operators on structures
----
---
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 #1172: THRIFT-4060 add better support in the cpp generator for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1172
Going to add the ability to generate with cpp:no_ostream_operators to omit definitions for ostream operators entirely (when set, declarations will still be made, which means consuming implementations must provide all the implementations just like they did before thrift-0.9.3).
---
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 #1172: THRIFT-4060 add better support in the cpp generat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1172
---
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 #1172: THRIFT-4060 add better support in the cpp generat...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1172#discussion_r98554132
--- Diff: compiler/cpp/src/thrift/generate/t_cpp_generator.cc ---
@@ -1117,7 +1129,7 @@ void t_cpp_generator::generate_struct_declaration(ofstream& out,
<< " &b);" << endl << endl;
}
- if (is_user_struct) {
+ if (is_user_struct && !has_custom_ostream(tstruct)) {
--- End diff --
This needs to change; on ostream declaration must be in the header even if thrift does not emit an implementation for it so that a thrift class that emits output code can contain a thrift class that does not.
---
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 #1172: THRIFT-4060 add better support in the cpp generat...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1172#discussion_r100583307
--- Diff: lib/cpp/test/AnnotationTest.cpp ---
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#define BOOST_TEST_MODULE AnnotationTest
+#include <boost/test/unit_test.hpp>
+#include "gen-cpp/AnnotationTest_types.h"
+#include <ostream>
+#include <sstream>
+
+// Normally thrift generates ostream operators, however
+// with the annotation "cpp.customostream" one can tell the
+// compiler they are going to provide their own, and not
+// emit operator << or printTo().
+
+std::ostream& operator<<(std::ostream& os, const ostr_custom& osc)
+{
+ os << "{ bar = " << osc.bar << "; }";
+ return os;
+}
+
+BOOST_AUTO_TEST_SUITE(BOOST_TEST_MODULE)
+
+BOOST_AUTO_TEST_CASE(test_cpp_customostream) {
+ //
+ // Show the compiler generated ostream operator does something..
+ //
+
+ ostr_default def;
+ def.__set_bar(10);
+
+ std::stringstream ssd;
+ ssd << def;
+ BOOST_CHECK_EQUAL(ssd.str(), "ostr_default(bar=10)");
+
+ //
+ // Now show that if cpp.customostream is added as an annotation,
+ // the behavior can be overridden in a safe and usable way (THRIFT-4060).
+ //
+
+ ostr_custom cus;
+ cus.__set_bar(10);
+
+ std::stringstream csd;
+ csd << cus;
+ BOOST_CHECK_EQUAL(csd.str(), "{ bar = 10; }");
--- End diff --
IMHO its better to keep "single test-case - single test" (or AAA or GivenWhenThen scheme). This file could include couple of SUITES (for example - one per annotation, and then well-named tests). Anytime I see comment I try to find a way to remade code so it does not need that comment ;)
---
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 #1172: THRIFT-4060 add better support in the cpp generat...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1172#discussion_r100707345
--- Diff: lib/cpp/test/AnnotationTest.cpp ---
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#define BOOST_TEST_MODULE AnnotationTest
+#include <boost/test/unit_test.hpp>
+#include "gen-cpp/AnnotationTest_types.h"
+#include <ostream>
+#include <sstream>
+
+// Normally thrift generates ostream operators, however
+// with the annotation "cpp.customostream" one can tell the
+// compiler they are going to provide their own, and not
+// emit operator << or printTo().
+
+std::ostream& operator<<(std::ostream& os, const ostr_custom& osc)
+{
+ os << "{ bar = " << osc.bar << "; }";
+ return os;
+}
+
+BOOST_AUTO_TEST_SUITE(BOOST_TEST_MODULE)
+
+BOOST_AUTO_TEST_CASE(test_cpp_customostream) {
+ //
+ // Show the compiler generated ostream operator does something..
+ //
+
+ ostr_default def;
+ def.__set_bar(10);
+
+ std::stringstream ssd;
+ ssd << def;
+ BOOST_CHECK_EQUAL(ssd.str(), "ostr_default(bar=10)");
+
+ //
+ // Now show that if cpp.customostream is added as an annotation,
+ // the behavior can be overridden in a safe and usable way (THRIFT-4060).
+ //
+
+ ostr_custom cus;
+ cus.__set_bar(10);
+
+ std::stringstream csd;
+ csd << cus;
+ BOOST_CHECK_EQUAL(csd.str(), "{ bar = 10; }");
--- End diff --
I split it out into two tests without comments.
---
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 #1172: THRIFT-4060 add better support in the cpp generator for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1172
So job #2 failed strangely in c_glib which is unrelated to this change.
I pushed an empty change to kick a rebuild. I expect it to clear...
Here's the job that failed:
https://travis-ci.org/apache/thrift/jobs/197100291
---
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.
---