Thrift: Move TStringBuffer functionality into TMemoryBuffer.
Summary:
TMemoryBuffer already has the necessary plubming to work with C++ strings.
This revision implements that functionality with a few wrapper methods.
Removed TStringBuffer as it should no longer be required (and it is tricky
to safely inherit from a class that has a non-virtual destructor).
Also refactored the TMemoryBuffer constructors a bit.
Reviewed By: aditya, yunfang
Test Plan:
test/TMemoryBufferTest.cpp
Revert Plan: ok
git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665213 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/lib/cpp/src/transport/TTransportUtils.cpp b/lib/cpp/src/transport/TTransportUtils.cpp
index 290b483..e5523be 100644
--- a/lib/cpp/src/transport/TTransportUtils.cpp
+++ b/lib/cpp/src/transport/TTransportUtils.cpp
@@ -190,7 +190,7 @@
return 0;
}
- // Device how much to give
+ // Decide how much to give
uint32_t give = len;
if (avail < len) {
give = avail;
@@ -203,6 +203,27 @@
return give;
}
+uint32_t TMemoryBuffer::readAppendToString(std::string& str, uint32_t len) {
+ // Check avaible data for reading
+ uint32_t avail = wPos_ - rPos_;
+ if (avail == 0) {
+ return 0;
+ }
+
+ // Device how much to give
+ uint32_t give = len;
+ if (avail < len) {
+ give = avail;
+ }
+
+ // Reserve memory, copy into string, and increment rPos_
+ str.reserve(str.length()+give);
+ str.append((char*)buffer_ + rPos_, give);
+ rPos_ += give;
+
+ return give;
+}
+
void TMemoryBuffer::write(const uint8_t* buf, uint32_t len) {
// Check available space
uint32_t avail = bufferSize_ - wPos_;
diff --git a/lib/cpp/src/transport/TTransportUtils.h b/lib/cpp/src/transport/TTransportUtils.h
index 102e848..e437ee9 100644
--- a/lib/cpp/src/transport/TTransportUtils.h
+++ b/lib/cpp/src/transport/TTransportUtils.h
@@ -8,7 +8,7 @@
#define _THRIFT_TRANSPORT_TTRANSPORTUTILS_H_ 1
#include <string>
-#include <sstream>
+#include <algorithm>
#include <transport/TTransport.h>
#include <transport/TFileTransport.h>
@@ -242,43 +242,59 @@
* @author Mark Slee <mcslee@facebook.com>
*/
class TMemoryBuffer : public TTransport {
- public:
- TMemoryBuffer() {
- owner_ = true;
- bufferSize_ = 1024;
- buffer_ = (uint8_t*)malloc(bufferSize_);
- if (buffer_ == NULL) {
- throw TTransportException("Out of memory");
+ private:
+
+ // Common initialization done by all constructors.
+ void initCommon(uint8_t* buf, uint32_t size, bool owner, uint32_t wPos) {
+ if (buf == NULL) {
+ assert(owner);
+ buf = (uint8_t*)malloc(size);
+ if (buf == NULL) {
+ throw TTransportException("Out of memory");
+ }
}
- wPos_ = 0;
+
+ buffer_ = buf;
+ bufferSize_ = size;
+ owner_ = owner;
+ wPos_ = wPos;
rPos_ = 0;
}
+ public:
+ static const uint32_t defaultSize = 1024;
+
+ TMemoryBuffer() {
+ initCommon(NULL, defaultSize, true, 0);
+ }
+
TMemoryBuffer(uint32_t sz) {
- owner_ = true;
- bufferSize_ = sz;
- buffer_ = (uint8_t*)malloc(bufferSize_);
- if (buffer_ == NULL) {
- throw TTransportException("Out of memory");
- }
- wPos_ = 0;
- rPos_ = 0;
+ initCommon(NULL, sz, true, 0);
}
- TMemoryBuffer(uint8_t* buf, int sz) {
- owner_ = false;
- buffer_ = buf;
- bufferSize_ = sz;
- wPos_ = sz;
- rPos_ = 0;
+ // transferOwnership should be true if you want TMemoryBuffer to call free(buf).
+ TMemoryBuffer(uint8_t* buf, int sz, bool transferOwnership = false) {
+ initCommon(buf, sz, transferOwnership, sz);
+ }
+
+ // copy should be true if you want TMemoryBuffer to make a copy of the string.
+ // If you set copy to false, the string must not be destroyed before you are
+ // done with the TMemoryBuffer.
+ TMemoryBuffer(const std::string& str, bool copy = false) {
+ if (copy) {
+ initCommon(NULL, str.length(), true, 0);
+ this->write(reinterpret_cast<const uint8_t*>(str.data()), str.length());
+ } else {
+ // This first argument should be equivalent to the following:
+ // const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(str.data()))
+ initCommon((uint8_t*)str.data(), str.length(), false, str.length());
+ }
}
~TMemoryBuffer() {
if (owner_) {
- if (buffer_ != NULL) {
- free(buffer_);
- buffer_ = NULL;
- }
+ free(buffer_);
+ buffer_ = NULL;
}
}
@@ -299,26 +315,56 @@
*sz = wPos_;
}
+ std::string getBufferAsString() {
+ return std::string((char*)buffer_, (std::string::size_type)bufferSize_);
+ }
+
+ void appendBufferToString(std::string& str) {
+ str.append((char*)buffer_, bufferSize_);
+ }
+
void resetBuffer() {
wPos_ = 0;
rPos_ = 0;
}
- void resetBuffer(uint8_t* buf, uint32_t sz) {
+ // transferOwnership should be true if you want TMemoryBuffer to call free(buf).
+ void resetBuffer(uint8_t* buf, uint32_t sz, bool transferOwnership = false) {
if (owner_) {
- if (buffer_ != NULL) {
- free(buffer_);
- }
+ free(buffer_);
}
- owner_ = false;
+ owner_ = donate;
buffer_ = buf;
bufferSize_ = sz;
wPos_ = sz;
rPos_ = 0;
}
+ // See the constructor that takes a string.
+ void resetFromString(const std::string& str, bool copy = false) {
+ if (copy) {
+ uint8_t* buf = (uint8_t*)malloc(str.length());
+ if (buf == NULL) {
+ throw TTransportException("Out of memory");
+ }
+ std::copy(str.begin(), str.end(), buf);
+ resetBuffer(buf, str.length(), true);
+ } else {
+ // See the above comment about const_cast.
+ resetBuffer((uint8_t*)str.data(), str.length(), false);
+ }
+ }
+
uint32_t read(uint8_t* buf, uint32_t len);
+ std::string readAsString(uint32_t len) {
+ std::string str;
+ (void)readAppendToString(str, len);
+ return str;
+ }
+
+ uint32_t readAppendToString(std::string& str, uint32_t len);
+
void readEnd() {
if (rPos_ == wPos_) {
resetBuffer();
@@ -331,83 +377,25 @@
return wPos_ - rPos_;
}
- protected:
+ private:
// Data buffer
uint8_t* buffer_;
// Allocated buffer size
uint32_t bufferSize_;
- // Is this object the owner of the buffer?
- bool owner_;
-
- private:
// Where the write is at
uint32_t wPos_;
// Where the reader is at
uint32_t rPos_;
+ // Is this object the owner of the buffer?
+ bool owner_;
};
/**
- * A string buffer is a tranpsort that simply reads from and writes to a
- * string. Anytime you call write on it, the data is serialized
- * into the underlying buffer, you can call getString() to get the serialized
- * string. Before you call read, you should call resetString(data) to set the
- * underlying buffer, you can then call read to get the
- * de-serialized data structure.
- *
- * The string buffer is inherited from the memory buffer
- * Thus, buffers are allocated using C constructs malloc,realloc, and the size
- * doubles as necessary.
- *
- * @author Yun-Fang Juan <yunfang@facebook.com>
- */
-
-class TStringBuffer : public TMemoryBuffer {
- public:
- TStringBuffer() : TMemoryBuffer() {};
-
- TStringBuffer(const std::string& inputString) : TMemoryBuffer() {
- resetString(inputString);
- };
-
- //destructor. basically the same as TMemoryBuffer
- ~TStringBuffer() {
- if (owner_) {
- if (buffer_ != NULL) {
- free(buffer_);
- buffer_ = NULL;
- }
- }
- }
-
- //from buffer to string
- std::string getString() {
- std::stringstream ss;
- ss.write((char*)buffer_, bufferSize_);
- return ss.str();
- };
-
- //from string to buffer
- void resetString(const std::string& inputString) {
- char* data;
- std::stringstream ss;
- bufferSize_ = inputString.size();
-
- ss.str(inputString);
- data = (char*)malloc(bufferSize_);
- ss.read(data, bufferSize_);
-
- resetBuffer((uint8_t*)data, bufferSize_);
- //set the owner_ to true so the buffer_ will be freed later on
- owner_ = true;
- };
-};
-
-/**
* TPipedTransport. This transport allows piping of a request from one
* transport to another either when readEnd() or writeEnd(). The typicAL
* use case for this is to log a request or a reply to disk.
diff --git a/test/DebugProtoTest.cpp b/test/DebugProtoTest.cpp
index 6c773a1..bae2a16 100644
--- a/test/DebugProtoTest.cpp
+++ b/test/DebugProtoTest.cpp
@@ -1,6 +1,6 @@
/*
thrift -cpp DebugProtoTest.thrift
-g++ -Wall -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
+g++ -Wall -g -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
DebugProtoTest.cpp gen-cpp/DebugProtoTest_types.cpp \
../lib/cpp/.libs/libthrift.a -o DebugProtoTest
./DebugProtoTest
diff --git a/test/DebugProtoTest.thrift b/test/DebugProtoTest.thrift
index 7510836..ac3b9b4 100644
--- a/test/DebugProtoTest.thrift
+++ b/test/DebugProtoTest.thrift
@@ -1,6 +1,6 @@
/*
thrift -cpp DebugProtoTest.thrift
-g++ -Wall -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
+g++ -Wall -g -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
DebugProtoTest.cpp gen-cpp/DebugProtoTest_types.cpp \
../lib/cpp/.libs/libthrift.a -o DebugProtoTest
./DebugProtoTest
diff --git a/test/OptionalRequiredTest.cpp b/test/OptionalRequiredTest.cpp
index 73574ee..643cbe9 100644
--- a/test/OptionalRequiredTest.cpp
+++ b/test/OptionalRequiredTest.cpp
@@ -1,6 +1,6 @@
/*
../compiler/cpp/thrift -cpp OptionalRequiredTest.thrift
-g++ -Wall -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
+g++ -Wall -g -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
OptionalRequiredTest.cpp gen-cpp/OptionalRequiredTest_types.cpp \
../lib/cpp/.libs/libthrift.a -o OptionalRequiredTest
./OptionalRequiredTest
diff --git a/test/OptionalRequiredTest.thrift b/test/OptionalRequiredTest.thrift
index 9738bd6..b2652dd 100644
--- a/test/OptionalRequiredTest.thrift
+++ b/test/OptionalRequiredTest.thrift
@@ -1,6 +1,6 @@
/*
../compiler/cpp/thrift -cpp OptionalRequiredTest.thrift
-g++ -Wall -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
+g++ -Wall -g -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
OptionalRequiredTest.cpp gen-cpp/OptionalRequiredTest_types.cpp \
../lib/cpp/.libs/libthrift.a -o OptionalRequiredTest
./OptionalRequiredTest
diff --git a/test/TMemoryBufferTest.cpp b/test/TMemoryBufferTest.cpp
new file mode 100644
index 0000000..a8975e1
--- /dev/null
+++ b/test/TMemoryBufferTest.cpp
@@ -0,0 +1,68 @@
+/*
+thrift -cpp ThriftTest.thrift
+g++ -Wall -g -I../lib/cpp/src -I/usr/local/include/boost-1_33_1 \
+ TMemoryBufferTest.cpp gen-cpp/ThriftTest_types.cpp \
+ ../lib/cpp/.libs/libthrift.a -o TMemoryBufferTest
+./TMemoryBufferTest
+*/
+
+#include <iostream>
+#include <climits>
+#include <cassert>
+#include <transport/TTransportUtils.h>
+#include <protocol/TBinaryProtocol.h>
+#include "gen-cpp/ThriftTest_types.h"
+
+
+int main(int argc, char** argv) {
+ {
+ using facebook::thrift::transport::TMemoryBuffer;
+ using facebook::thrift::protocol::TBinaryProtocol;
+ using boost::shared_ptr;
+
+ shared_ptr<TMemoryBuffer> strBuffer(new TMemoryBuffer());
+ shared_ptr<TBinaryProtocol> binaryProtcol(new TBinaryProtocol(strBuffer));
+
+ thrift::test::Xtruct a;
+ a.i32_thing = 10;
+ a.i64_thing = 30;
+ a.string_thing ="holla back a";
+
+ a.write(binaryProtcol.get());
+ std::string serialized = strBuffer->getBufferAsString();
+
+ shared_ptr<TMemoryBuffer> strBuffer2(new TMemoryBuffer());
+ shared_ptr<TBinaryProtocol> binaryProtcol2(new TBinaryProtocol(strBuffer2));
+
+ strBuffer2->resetFromString(serialized);
+ thrift::test::Xtruct a2;
+ a2.read(binaryProtcol2.get());
+
+ assert(a == a2);
+ }
+
+ {
+ using facebook::thrift::transport::TMemoryBuffer;
+ using std::string;
+ using std::cout;
+ using std::endl;
+
+ string* str1 = new string("abcd1234");
+ const char* data1 = str1->data();
+ TMemoryBuffer buf(*str1, true);
+ delete str1;
+ string* str2 = new string("plsreuse");
+ bool obj_reuse = (str1 == str2);
+ bool dat_reuse = (data1 == str2->data());
+ cout << "Object reuse: " << obj_reuse << " Data reuse: " << dat_reuse
+ << ((obj_reuse && dat_reuse) ? " YAY!" : "") << endl;
+ delete str2;
+
+ string str3 = "wxyz", str4 = "6789";
+ buf.readAppendToString(str3, 4);
+ buf.readAppendToString(str4, INT_MAX);
+
+ assert(str3 == "wxyzabcd");
+ assert(str4 == "67891234");
+ }
+}