You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/06/14 16:43:57 UTC
[GitHub] [incubator-tvm] zhiics opened a new pull request #5806: [RUNTIME][String] Overload string operators
zhiics opened a new pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806
Per #5792
@tqchen @junrushao1994
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440328106
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
yes, you are right.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen merged pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440275128
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
We will need to have a customized deleter. Directly allocating StringObj will cause the memory not being released (because the deleter won't recycle the data in the StringObj). Let us use StringObj::FromStd for now. Alternatively, create another subclass of StringObj that contains the customized deleter(hopefully via inplaceArray)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440309404
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
There is no dispatch needed because all operations only differs in creation time
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r439874434
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
+ ptr->size = lhs_size + rhs_size;
+ ptr->data = concat;
+ return String(ptr);
+}
+
+inline String operator+(const char* lhs, const String& rhs) {
+ size_t lhs_size = std::strlen(lhs);
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size];
+ std::memcpy(concat, lhs, lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
+ ptr->size = lhs_size + rhs_size;
+ ptr->data = concat;
+ return String(ptr);
}
+inline String operator+(const String& lhs, const char* rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = std::strlen(rhs);
+ char* concat = new char[lhs_size + rhs_size];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs, rhs_size);
+ auto ptr = make_object<StringObj>();
+ ptr->size = lhs_size + rhs_size;
+ ptr->data = concat;
+ return String(ptr);
+}
+
+// Overload < operator
+inline bool operator<(const String& lhs, const std::string& rhs) { return lhs.compare(rhs) < 0; }
+
+inline bool operator<(const std::string& lhs, const String& rhs) { return rhs.compare(lhs) > 0; }
+
+inline bool operator<(const String& lhs, const String& rhs) { return lhs.compare(rhs) < 0; }
+
+inline bool operator<(const String& lhs, const char* rhs) { return lhs.compare(rhs) < 0; }
+
+inline bool operator<(const char* lhs, const String& rhs) { return rhs.compare(lhs) > 0; }
+
+inline bool operator>(const String& lhs, const std::string& rhs) { return lhs.compare(rhs) > 0; }
Review comment:
move this two lines downside
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
+ ptr->size = lhs_size + rhs_size;
+ ptr->data = concat;
+ return String(ptr);
+}
+
+inline String operator+(const char* lhs, const String& rhs) {
+ size_t lhs_size = std::strlen(lhs);
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size];
Review comment:
Shall we add a `\0` to the end of the string?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440275128
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
We will need to have a customized deleter. Directly allocating will cause the memory not being released. Let us use StringObj::FromStd for now. Alternatively, create another subclass of StringObj that contains the customized deleter(hopefully via inplaceArray)
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
+ ptr->size = lhs_size + rhs_size;
+ ptr->data = concat;
+ return String(ptr);
+}
+
+inline String operator+(const char* lhs, const String& rhs) {
+ size_t lhs_size = std::strlen(lhs);
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs, lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
Review comment:
Createa common private static function, Concat(lhs, size, rhs, size)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r441117455
##########
File path: include/tvm/runtime/container.h
##########
@@ -1372,6 +1306,31 @@ class String : public ObjectRef {
*/
static int memncmp(const char* lhs, const char* rhs, size_t lhs_count, size_t rhs_count);
+ /*!
+ * \brief Concatenate two char sequences
+ *
+ * \param lhs Pointers to the lhs char array
+ * \param lhs_size The size of the lhs char array
+ * \param rhs Pointers to the rhs char array
+ * \param rhs_size The size of the rhs char array
+ *
+ * \return The concatenated char sequence
+ */
+ static char* Concat(const char* lhs, size_t lhs_size, const char* rhs, size_t rhs_size) {
+ char* concat = new char[lhs_size + rhs_size + 1];
Review comment:
This does not resolve the problem of memory leak(since the new char did not get deleted. What I actually meant is to make Concat directly return a String instead, and inside here we use std::string for the space backup.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#issuecomment-643832486
Nice work :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440610437
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1369,107 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+inline String operator+(const String& lhs, const String& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
}
+inline String operator+(const String& lhs, const std::string& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const std::string& lhs, const String& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const char* lhs, const String& rhs) {
+ size_t lhs_size = std::strlen(lhs);
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs, lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const String& lhs, const char* rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = std::strlen(rhs);
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs, rhs_size);
+ return String(concat);
+}
+
+// Overload < operator
+inline bool operator<(const String& lhs, const std::string& rhs) { return lhs.compare(rhs) < 0; }
Review comment:
@zhiics : Thanks for the PR! The changes looks good to me. Great work:)
One small suggestion(Not Important Though).
May be we can use some Macro as below:
OPERATOR_OVERLOAD_COMMUTATIVE(operator<, String, std::string)
#define OPERATOR_OVERLOAD_COMMUTATIVE(OP, T1, T2) \
inline bool OP(const T1& lhs, const T2& rhs) { return lhs.compare(rhs) < 0; } \
inline bool OP(const T2& lhs, const T1& rhs) { return rhs.compare(lhs) > 0; }
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r441138133
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1369,107 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+inline String operator+(const String& lhs, const String& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
}
+inline String operator+(const String& lhs, const std::string& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const std::string& lhs, const String& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const char* lhs, const String& rhs) {
+ size_t lhs_size = std::strlen(lhs);
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs, lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const String& lhs, const char* rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = std::strlen(rhs);
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs, rhs_size);
+ return String(concat);
+}
+
+// Overload < operator
+inline bool operator<(const String& lhs, const std::string& rhs) { return lhs.compare(rhs) < 0; }
Review comment:
Thanks for suggestion. I would keep the current form as there are already one-liners.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r441138133
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1369,107 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+inline String operator+(const String& lhs, const String& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
}
+inline String operator+(const String& lhs, const std::string& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const std::string& lhs, const String& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const char* lhs, const String& rhs) {
+ size_t lhs_size = std::strlen(lhs);
+ size_t rhs_size = rhs.size();
+ char* concat = String::Concat(lhs, lhs_size, rhs.data(), rhs_size);
+ return String(concat);
+}
+
+inline String operator+(const String& lhs, const char* rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = std::strlen(rhs);
+ char* concat = String::Concat(lhs.data(), lhs_size, rhs, rhs_size);
+ return String(concat);
+}
+
+// Overload < operator
+inline bool operator<(const String& lhs, const std::string& rhs) { return lhs.compare(rhs) < 0; }
Review comment:
Thanks for suggestion. I would keep the current form as they are already one-liners.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440275128
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
We will need to have a customized deleter. Directly allocating StringObj will cause the memory not being released (because the deleter won't recycle the data in the StringObj). Let us use StringObj::FromStd for now. Alternatively, create another subclass of StringObj that contains the customized deleter(hopefully via InplaceArray, to further reduce indirection) cc @junrushao1994
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440277395
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
Review comment:
The match is a bit too broad, let us still directly enumerate the combinations that involves String
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r441138644
##########
File path: include/tvm/runtime/container.h
##########
@@ -1372,6 +1306,31 @@ class String : public ObjectRef {
*/
static int memncmp(const char* lhs, const char* rhs, size_t lhs_count, size_t rhs_count);
+ /*!
+ * \brief Concatenate two char sequences
+ *
+ * \param lhs Pointers to the lhs char array
+ * \param lhs_size The size of the lhs char array
+ * \param rhs Pointers to the rhs char array
+ * \param rhs_size The size of the rhs char array
+ *
+ * \return The concatenated char sequence
+ */
+ static char* Concat(const char* lhs, size_t lhs_size, const char* rhs, size_t rhs_size) {
+ char* concat = new char[lhs_size + rhs_size + 1];
Review comment:
Sorry. I misunderstood.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440295471
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
We should
1) mark `make_object<StringObj>` as deleted, in case there is any misuse in the codebase
2) have a separate subclass of StringObj to make it clearer
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440483807
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
Let us use FromStd for now. The customized deleter is an incremental change by changing the constructor of `String(const char*)` which needs some extra efforts
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440329966
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
we can probably have a `StringObj::FromCharPtr` for `const char*` and make the constructor use `make_inplace_array`. Currently we just use `std::string` for the `const char*`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request #5806: [RUNTIME][String] Overload string operators
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5806:
URL: https://github.com/apache/incubator-tvm/pull/5806#discussion_r440296478
##########
File path: include/tvm/runtime/container.h
##########
@@ -1410,10 +1344,114 @@ inline String& String::operator=(std::string other) {
inline String& String::operator=(const char* other) { return operator=(std::string(other)); }
-inline String operator+(const std::string lhs, const String& rhs) {
- return lhs + rhs.operator std::string();
+template <typename T, typename U,
+ typename = typename std::enable_if<std::is_same<T, String>::value ||
+ std::is_same<T, std::string>::value>::type,
+ typename = typename std::enable_if<std::is_same<U, String>::value ||
+ (std::is_same<U, std::string>::value &&
+ !std::is_same<T, U>::value)>::type>
+inline String operator+(const T& lhs, const U& rhs) {
+ size_t lhs_size = lhs.size();
+ size_t rhs_size = rhs.size();
+ char* concat = new char[lhs_size + rhs_size + 1];
+ std::memcpy(concat, lhs.data(), lhs_size);
+ std::memcpy(concat + lhs_size, rhs.data(), rhs_size);
+ auto ptr = make_object<StringObj>();
Review comment:
In my implementation of runtime::Array, I overload `make_object<ArrayNode>` to `make_inplace_array`; In Map, I dispatch `make_object<MapNode>` to `make_object<SmallMapNode>`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org