You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by beberg <gi...@git.apache.org> on 2015/10/14 21:46:42 UTC
[GitHub] thrift pull request: THRIFT-3382 TBase class for C++ Library
GitHub user beberg opened a pull request:
https://github.com/apache/thrift/pull/653
THRIFT-3382 TBase class for C++ Library
Add a TBase that all Struct's use as their base class.
Contributed by Sentient Technologies - http://www.sentient.ai/
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/beberg/thrift THRIFT-3382-TBase-for-cpp
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/653.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 #653
----
commit cc9002a82229cc680a21c8f749a5a21dc2e4e760
Author: Adam Beberg <be...@sentient.ai>
Date: 2015-09-23T21:50:01Z
Support for database/sql - Scan(), Value(), and db: tags
Contributed by Sentient Technologies - http://www.sentient.ai/
commit 713230d6b8ebdbbb6f7e9991b7f932fa4138e03e
Author: Adam Beberg <be...@mithral.com>
Date: 2015-09-25T18:45:45Z
Merge pull request #1 from beberg/THRIFT-3339-Go-Support-for-Databases
Support for database/sql - Scan(), Value(), and db: tags
commit 61010d66c38dc0b086e8aedc13f424eaf6d75aeb
Author: Adam Beberg <be...@sentient.ai>
Date: 2015-09-26T01:18:56Z
Fix error message text
commit d9534313798e03656db7eb407b3332a89ca2cb0e
Author: Adam Beberg <be...@mithral.com>
Date: 2015-09-26T01:20:23Z
Merge pull request #2 from beberg/THRIFT-3339-Go-Support-for-Databases
Fix error message text
commit 4d6288dfa681d33020ffa40ed675b2fc68f393aa
Author: Adam Beberg <be...@sentient.ai>
Date: 2015-09-23T21:50:01Z
Support for database/sql - Scan(), Value(), and db: tags
Contributed by Sentient Technologies - http://www.sentient.ai/
commit a671c0097dfd04c58563c2075c474aec2b134f35
Author: Adam Beberg <be...@sentient.ai>
Date: 2015-09-26T01:18:56Z
Fix error message text
commit 474331f4e845e65927879af4f8b8c05680333911
Author: Adam Beberg <be...@sentient.ai>
Date: 2015-10-14T17:31:20Z
Merge branch 'master' of https://github.com/beberg/thrift
commit 669a583cc360528d922d7f395a13448c836a3786
Author: Adam Beberg <be...@sentient.ai>
Date: 2015-10-14T18:37:39Z
THRIFT-3382 TBase class for C++ Library
Add a TBase that all Struct's use as their base class.
Contributed by Sentient Technologies - http://www.sentient.ai/
----
---
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: THRIFT-3382 TBase class for C++ Library
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/653#discussion_r42043446
--- Diff: compiler/cpp/src/generate/t_cpp_generator.cc ---
@@ -116,7 +116,8 @@ class t_cpp_generator : public t_oop_generator {
bool read = true,
bool write = true,
bool swap = false,
- bool stream = false);
+ bool stream = false,
+ bool is_struct = false);
--- End diff --
is this necessary? it's either struct or exception, and there is already bool for that. or am I missing something? IMHO there is too many of those bools already :)
---
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: THRIFT-3382 TBase class for C++ Library
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/653#discussion_r42045161
--- Diff: compiler/cpp/src/generate/t_cpp_generator.cc ---
@@ -116,7 +116,8 @@ class t_cpp_generator : public t_oop_generator {
bool read = true,
bool write = true,
bool swap = false,
- bool stream = false);
+ bool stream = false,
+ bool is_struct = false);
--- End diff --
couple of solutions:
- convert some of those bools into enum (especially that is_exception=true and is_struct=true can be set but it's a nonsense) [imho preferred]
- rename `bool stream` to `is_not_internal` or something like that (it was also added to distinguish internal structs and normal ones)
- do not bother that internal structs inherit from TBase, they're structs anyway
---
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: THRIFT-3382 TBase class for C++ Library
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/653#discussion_r42043448
--- Diff: lib/cpp/src/thrift/TBase.h ---
@@ -0,0 +1,36 @@
+/*
+ * 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.
+ */
+
+#ifndef _THRIFT_TBASE_H_
+#define _THRIFT_TBASE_H_ 1
+
+namespace apache {
+namespace thrift {
+
+class TBase {
+public:
+ virtual ~TBase(){};
+ virtual uint32_t read(::apache::thrift::protocol::TProtocol* iprot) = 0;
--- End diff --
this header is not self contained - it needs uint32_t and TProtocol definitions (for second thing - at least forward decl)
---
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: THRIFT-3382 TBase class for C++ Library
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/653
---
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: THRIFT-3382 TBase class for C++ Library
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/653#discussion_r42060020
--- Diff: lib/cpp/src/thrift/TBase.h ---
@@ -0,0 +1,39 @@
+/*
+ * 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.
+ */
+
+#ifndef _THRIFT_TBASE_H_
+#define _THRIFT_TBASE_H_ 1
+
+#include <sys/types.h>
--- End diff --
uint32_t in Thrift comes from Thrift.h - not all platforms have sys/types.h etc, so Thrift.h contains some ifdefs etc
---
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.
---