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/01/06 05:19:27 UTC
[GitHub] [incubator-tvm] wweic opened a new pull request #4628: [Object] Add
String container
wweic opened a new pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628
cc @tqchen @icemelon9 @FrozenGene
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r373981359
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
Review comment:
Making it rvalue will require user of the API to always move other, making it value will enable coping `other` if user wants to keep 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554812
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = other;
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const { return operator->()->size; }
Review comment:
Compiler should be clever enough most of time, so we can just use inline for now
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r387973349
##########
File path: tests/cpp/container_test.cc
##########
@@ -221,11 +222,154 @@ TEST(Map, Iterator) {
using namespace tvm;
PrimExpr a = 1, b = 2;
Map<PrimExpr, PrimExpr> map1{{a, b}};
- std::unordered_map<PrimExpr, PrimExpr, ObjectHash, ObjectEqual>
- map2(map1.begin(), map1.end());
+ std::unordered_map<PrimExpr, PrimExpr, ObjectHash, ObjectEqual> map2(
+ map1.begin(), map1.end());
CHECK(map2[a].as<IntImmNode>()->value == 2);
}
+TEST(String, MoveFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s(std::move(source));
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), 0);
+}
+
+TEST(String, CopyFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s{source};
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), expect.size());
+}
+
+TEST(String, Assignment) {
+ using namespace std;
+ String s{string{"hello"}};
+ s = string{"world"};
+ CHECK_EQ(s == "world", true);
+ string s2{"world2"};
+ s = std::move(s2);
+ CHECK_EQ(s == "world2", true);
+}
+
+TEST(String, empty) {
+ using namespace std;
+ String s{"hello"};
+ CHECK_EQ(s.empty(), false);
+ s = "";
+ CHECK_EQ(s.empty(), true);
+}
+
+TEST(String, Comparisons) {
+ using namespace std;
+ string source = "a string";
+ string mismatch = "a string but longer";
+ String s{source};
+
+ CHECK_EQ(s == source, true);
+ CHECK_EQ(s == mismatch, false);
+ CHECK_EQ(s == source.data(), true);
+ CHECK_EQ(s == mismatch.data(), false);
+
+ // Check '\0' handling
+ string v1 = "hello world";
+ size_t v1_size = v1.size();
+ v1[5] = '\0';
+ CHECK_EQ(v1[5], '\0');
+ CHECK_EQ(v1.size(), v1_size);
Review comment:
Need to add test coverage to test cases that would have been different if the str is defined to terminate at \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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376752375
##########
File path: include/tvm/runtime/container.h
##########
@@ -24,6 +24,19 @@
#ifndef TVM_RUNTIME_CONTAINER_H_
#define TVM_RUNTIME_CONTAINER_H_
+#if defined(__cpp_lib_experimental_string_view) && \
+ __cpp_lib_experimental_string_view >= 201411
+#define TVM_USE_CXX14_STRING_VIEW
+#elif defined(__cpp_lib_string_view) && __cpp_lib_string_view >= 201606
+#define TVM_USE_CXX17_STRING_VIEW
Review comment:
We shall prioritize CXX17 string view as it is the std. also when if we defint it this way, it can leave certain macro undefined. I think we should still define it in the old way as given in the example so that the macro is always defined
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-597934612
@tqchen Yes. I'll explore Python interface for 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r387972834
##########
File path: tests/cpp/container_test.cc
##########
@@ -221,11 +222,154 @@ TEST(Map, Iterator) {
using namespace tvm;
PrimExpr a = 1, b = 2;
Map<PrimExpr, PrimExpr> map1{{a, b}};
- std::unordered_map<PrimExpr, PrimExpr, ObjectHash, ObjectEqual>
- map2(map1.begin(), map1.end());
+ std::unordered_map<PrimExpr, PrimExpr, ObjectHash, ObjectEqual> map2(
+ map1.begin(), map1.end());
CHECK(map2[a].as<IntImmNode>()->value == 2);
}
+TEST(String, MoveFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s(std::move(source));
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), 0);
+}
+
+TEST(String, CopyFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s{source};
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), expect.size());
+}
+
+TEST(String, Assignment) {
+ using namespace std;
+ String s{string{"hello"}};
+ s = string{"world"};
+ CHECK_EQ(s == "world", true);
+ string s2{"world2"};
+ s = std::move(s2);
+ CHECK_EQ(s == "world2", true);
+}
+
+TEST(String, empty) {
+ using namespace std;
+ String s{"hello"};
+ CHECK_EQ(s.empty(), false);
+ s = "";
+ CHECK_EQ(s.empty(), true);
+}
+
+TEST(String, Comparisons) {
+ using namespace std;
+ string source = "a string";
+ string mismatch = "a string but longer";
+ String s{source};
+
+ CHECK_EQ(s == source, true);
+ CHECK_EQ(s == mismatch, false);
+ CHECK_EQ(s == source.data(), true);
+ CHECK_EQ(s == mismatch.data(), false);
+
+ // Check '\0' handling
+ string v1 = "hello world";
+ size_t v1_size = v1.size();
+ v1[5] = '\0';
+ CHECK_EQ(v1[5], '\0');
+ CHECK_EQ(v1.size(), v1_size);
+ String str_v1{v1};
+ CHECK_EQ(str_v1 == v1, true);
+ CHECK_EQ(str_v1.size(), v1_size);
+
+ string v2 = "hello";
+ CHECK_EQ(str_v1 == v2, false);
+}
+
+TEST(String, compare) {
+ using namespace std;
+ string source = "a string";
+ string mismatch1 = "a string but longer";
+ string mismatch2 = "a strin";
+ string mismatch3 = "a b";
+ string mismatch4 = "a t";
+ String str_source{source};
+ String str_mismatch1{mismatch1};
+ String str_mismatch2{mismatch2};
+ String str_mismatch3{mismatch3};
+ String str_mismatch4{mismatch4};
+
+ // compare with string
+ CHECK_EQ(str_source.compare(source), 0);
+ CHECK_LT(str_source.compare(mismatch1), 0);
+ CHECK_GT(str_source.compare(mismatch2), 0);
+ CHECK_GT(str_source.compare(mismatch3), 0);
+ CHECK_LT(str_source.compare(mismatch4), 0);
+
+ // compare with char*
+ CHECK_EQ(str_source.compare(source.data()), 0);
+ CHECK_LT(str_source.compare(mismatch1.data()), 0);
+ CHECK_GT(str_source.compare(mismatch2.data()), 0);
+ CHECK_GT(str_source.compare(mismatch3.data()), 0);
+ CHECK_LT(str_source.compare(mismatch4.data()), 0);
+
+ // compare with String
+ CHECK_LT(str_source.compare(str_mismatch1), 0);
+ CHECK_GT(str_source.compare(str_mismatch2), 0);
+ CHECK_GT(str_source.compare(str_mismatch3), 0);
+ CHECK_LT(str_source.compare(str_mismatch4), 0);
+
+ // Check '\0' handling
+ string v1 = "hello world";
Review comment:
We need broader coverage for \0 handling. In particular, we should construct mismatch cases that would have been equal to each other if we use strncmp
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] yzhliu commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
yzhliu commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-593691541
good to me.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374448792
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
Review comment:
\return the comparison result
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386066199
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
Review comment:
@tqchen Thanks! I have changed the interface accordingly.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-590977222
Interesting, i think it is fine given that we will fallback to the std::string impl. The standard only requires it to available in C++17, so they might indeed be optional in the case of c++14
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-596871935
ping @wweic :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800596
##########
File path: include/tvm/runtime/container.h
##########
@@ -24,11 +24,35 @@
#ifndef TVM_RUNTIME_CONTAINER_H_
#define TVM_RUNTIME_CONTAINER_H_
+// Reference for feature test macros of string_view:
+// https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations
+// https://en.cppreference.com/w/User:D41D8CD98F/feature_testing_macros
+#if defined(__cpp_lib_experimental_string_view) && \
+ __cpp_lib_experimental_string_view >= 201411
+#define TVM_USE_CXX14_STRING_VIEW 1
Review comment:
chang to TVM_USE_CXX14_STING_VIEW_HASH
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363405674
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
Review comment:
+1, since we want string to be immutable
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-584097003
> @FrozenGene In general I agree that we should avoid std::experimental.
>
> In this particular case, i think the usage is fair, because it is guarded by marco tests and is only under a very limited case we a std::hash function that can hash a string without copying it(instead of using the string_view data structure).
>
> * T0: We could have implemented a hash function by ourselves, but the hash itself may be inconsistent with the std version.
> * T1: While the std::experimental::string_view's hash could have been inconsistent with the std::string version as per compiler version(because of experimental), in practice it is consistent with std::string as per string_view proposal(and can be confirmed using different compilers). More importantly, it is also fine if the hash is inconsistent with the std ver(then we will be the case of T1.
>
> Given the above consideration, I think it is fine to permit the limited usecase. However, I agree that we should have a more careful documentation about the std::experimental use case here and only limit it to the specific usecase.
Ok. One minor comment: let us add one comment in the C++14 experimental part what compiler (version) we have tested. Like GCC 5.4 / Clang 7.0 etc.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4628: [Object]
Add String container
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583765522
@wweic here is an code snippet that we can reuse for hash. We should consider migrate to require c++14, which will give us string view support
```c++
#define TVM_USE_CXX14_STRING_VIEW \
defined(__cpp_lib_experimental_string_view) && __cpp_lib_experimental_string_view >= 201411
#define TVM_USE_CXX17_STRING_VIEW \
defined(__cpp_lib_string_view) && __cpp_lib_string_view >= 201606
#include <string>
#include <dmlc/logging.h>
#if TVM_USE_CXX17_STRING_VIEW
#include <string_view>
#elif TVM_USE_CXX14_STRING_VIEW
#include <experimental/string_view>
#endif
int main(int argc, char* argv[]) {
std::string xyz = "xyz";
#if TVM_USE_CXX17_STRING_VIEW
LOG(INFO) << "C++17=" << std::hash<std::string_view>()(xyz);
#elif TVM_USE_CXX14_STRING_VIEW
LOG(INFO) << "C++14=" << std::hash<std::experimental::string_view>()(xyz);
#else
LOG(INFO) << "C++11=" << std::hash<std::string>()(xyz);
#endif
return 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376696045
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const { return get()->data; }
+
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+}
+
} // namespace runtime
} // namespace tvm
+namespace std {
+
+template <>
+struct hash<::tvm::runtime::String> {
+ std::size_t operator()(const ::tvm::runtime::String& str) const {
+ return std::hash<std::string>()((std::string)str);
Review comment:
How about we cache the hash value of `std::hash` in `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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376802418
##########
File path: tests/cpp/container_test.cc
##########
@@ -226,6 +226,94 @@ TEST(Map, Iterator) {
CHECK(map2[a].as<IntImmNode>()->value == 2);
}
+TEST(String, MoveFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s(std::move(source));
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), 0);
+}
+
+TEST(String, CopyFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s{source};
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), expect.size());
+}
+
+TEST(String, Assignment) {
+ using namespace std;
+ String s{string{"hello"}};
+ s = string{"world"};
+ CHECK_EQ(s == "world", true);
+ string s2{"world2"};
+ s = std::move(s2);
+ CHECK_EQ(s == "world2", true);
+}
+
+TEST(String, Comparisons) {
+ using namespace std;
+ string source = "a string";
+ string mismatch = "a string but longer";
+ String s{source};
+
+ CHECK_EQ(s == source, true);
+ CHECK_EQ(s == mismatch, false);
+ CHECK_EQ(s == source.data(), true);
+ CHECK_EQ(s == mismatch.data(), false);
+
+ // Check '\0' handling
+ string v1 = "hello world";
Review comment:
Add compare string with different length
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374447406
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
Review comment:
Perhaps we can consider assign a fixed index once we stablize the string obj
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] icemelon9 commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365619982
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
Review comment:
```suggestion
inline String::String(std::string&& other) {
```
I think using rvalue ref here is safer as `other` will be cleared in this constructor.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374448684
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
Review comment:
Add a fast path, when the const char* pointer equals each other, return 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] icemelon9 commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374229865
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "vm.String";
Review comment:
Maybe just call it String, since this won't be specific to vm.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376752387
##########
File path: include/tvm/runtime/container.h
##########
@@ -457,6 +470,26 @@ class String : public ObjectRef {
*/
operator std::string() const { return std::string{get()->data, size()}; }
+#if TVM_USE_CXX17_STRING_VIEW
+ /*!
+ * \brief Convert String to an std::string_view object
+ *
+ * \return std::string_view
+ */
+ operator std::string_view() const {
+ return std::string_view{get()->data, size()};
+ }
+#elif TVM_USE_CXX14_STRING_VIEW
+ /*!
+ * \brief Convert String to an std::experimental::string_view object
+ *
+ * \return std::experimental::string_view
+ */
+ operator std::experimental::string_view() const {
+ return std::experimental::string_view{get()->data, size()};
Review comment:
I am not to sure if we want to convert to experimental string_view, as it is not a part of std yet
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374449910
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const { return get()->data; }
+
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
Review comment:
Consider make it private, as it is used internally.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4628: [Object]
Add String container
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583765522
@wweic here is a code snippet that we can reuse for hash. We should consider migrate to require c++14, which will give us string view support
```c++
#define TVM_USE_CXX14_STRING_VIEW \
defined(__cpp_lib_experimental_string_view) && __cpp_lib_experimental_string_view >= 201411
#define TVM_USE_CXX17_STRING_VIEW \
defined(__cpp_lib_string_view) && __cpp_lib_string_view >= 201606
#include <string>
#include <dmlc/logging.h>
#if TVM_USE_CXX17_STRING_VIEW
#include <string_view>
#elif TVM_USE_CXX14_STRING_VIEW
#include <experimental/string_view>
#endif
int main(int argc, char* argv[]) {
std::string xyz = "xyz";
#if TVM_USE_CXX17_STRING_VIEW
LOG(INFO) << "C++17=" << std::hash<std::string_view>()(xyz);
#elif TVM_USE_CXX14_STRING_VIEW
LOG(INFO) << "C++14=" << std::hash<std::experimental::string_view>()(xyz);
#else
LOG(INFO) << "C++11=" << std::hash<std::string>()(xyz);
#endif
return 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374448310
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
Review comment:
NOTE: perhaps it worth to add a testcase
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen merged pull request #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363408036
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = other;
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
Review comment:
let use add common methods for std::string
- length- compare
- operator==(std::string) !=
- operator==(const char*) !=
- c_str()
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-591453421
@wweic mac is libcxx. According to the doc : https://libcxx.llvm.org/docs/FeatureTestMacroTable.html it should have __cpp_lib_string_view in C++17. We should make C++17 could work. C++14 doesn’t have macro to guarantee
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-588607759
@tqchen I'm working on the comments :-) Will update soon.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r387974187
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,286 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered_map<String, int32_t> m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return this->compare(other) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return compare(other) == 0; }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The String to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The string to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this to other
+ *
+ * \param other The character array to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other) const {
+ if (other == data()) {
+ return 0;
+ }
+ return memncmp(data(), other, size(), std::strlen(other));
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Retun if the string is empty
+ *
+ * \return true if empty, false otherwise.
+ */
+ bool empty() const { return size() == 0; }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Compare two char sequence
+ *
+ * \param lhs Pointers to the char array to compare
+ * \param rhs Pointers to the char array to compare
+ * \param lhs_count Length of the char array to compare
+ * \param rhs_count Length of the char array to compare
+ * \return int zero if both char sequences compare equal. negative if this
+ * appear before other, positive otherwise.
+ */
+ static int memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) : data_container{other} {}
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other) {
+ String replace{std::move(other)};
+ data_.swap(replace.data_);
+ return Downcast<String>(*this);
+}
+
+inline int String::memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count) {
+ for (size_t i = 0; i < lhs_count && i < rhs_count; ++i) {
Review comment:
Add an optimization
```
if (lhs == rhs && lhs_count == rhs_count) return 0;
```
Add test to cover this case
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801571
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
Review comment:
use uint64_t here.
Rationale: we will likely need to store big binary blob that goes beyond 32 bits. Because of the memory alignment, uint32_t won't save us more memory here
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374447314
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
Review comment:
NOTE: under the new convention, likely we will name it as `runtime.String` as type key
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365605959
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return operator->()->size != std::strlen(other) ||
+ std::strncmp(operator->()->data, other, operator->()->size) != 0;
Review comment:
We don;t have to do strlen. strncmp should be suffice here
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-573367749
@tqchen @FrozenGene Thanks for the comments. Please take a look again.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801581
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
Review comment:
consider put data before 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554643
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
Review comment:
NOTE: operator== should always take const reference (const std::string&) otehrwise it will results in a copy
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374448205
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
Review comment:
I am not sure strncmp is the right way to do it, esepcially when there is a case of `\0` in the string. Use http://www.cplusplus.com/reference/cstring/memcmp/ instead. See spec http://www.cplusplus.com/reference/string/string/compare/
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4628: [Object]
Add String container
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583871162
@FrozenGene In general I agree that we should avoid std::experimental.
In this particular case, i think the usage is fair, because it is guarded by marco tests and is only under a very limited case we a std::hash function that can hash a string without copying it(instead of using the string_view data structure).
- T0: We could have implemented a hash function by ourselves, but the hash itself may be inconsistent with the std version.
- T1: While the std::experimental::string_view's hash could have been inconsistent with the std::string version as per compiler version(because of experimental), in practice it is consistent with std::string as per string_view proposal(and can be confirmed using different compilers). More importantly, it is also fine if the hash is inconsistent with the std ver(then we will be the case of T1.
Given the above consideration, I think it is fine to permit the limited usecase. However, I agree that we should have a more careful documentation about the std::experimental use case here and only limit it to the specific usecase.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4628: [Object]
Add String container
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-584734563
@wweic please update per comments :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363405855
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = other;
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const { return operator->()->size; }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return char* data pointer
+ */
+ char* data() const { return operator->()->data; }
+
+ /*!
+ * \brief Create a String reference from std::string rvalue
+ *
+ * \param other The source std::string object.
+ * \return String The String reference.
+ */
+ static String FromStd(std::string&& other) {
+ return String(std::move(other));
+ }
+
+ /*!
+ * \brief Create a String reference from std::string reference
+ *
+ * \param other The source std::string object.
+ * \return String The String reference.
+ */
+ static String FromStd(const std::string& other) { return String(other); }
Review comment:
Given that we already have constructor, we don't need these functions
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363406117
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
Review comment:
mark it as inline and move constructors to inline sessions.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-573450243
we can do it in a followup PR but as a reminder, we need python side wrapper for String and perhaps we need to make some special treatment to make sure python could treat it as customized string types (e.g. by subclass string or related).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363407249
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
Review comment:
define ```operator=(std::string)``` using https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Copy-and-swap
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386213213
##########
File path: python/tvm/relay/prelude.py
##########
@@ -26,6 +26,42 @@
from . import op
+class StaticTensorArrayOps(object):
Review comment:
How is this related?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363407083
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
Review comment:
We only need to define a constructor, and it should cover the move and copy constructors(when const parameter is passed, it will trigger a copy, otherwise a move.
```c++
explicit String(std::string other) {
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386077522
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,277 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return this->compare(other) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return compare(other) == 0; }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The String to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The string to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this to other
+ *
+ * \param other The character array to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other) const {
+ if (other == data()) {
+ return 0;
+ }
+ return memncmp(data(), other, size(), std::strlen(other));
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Compare two char sequence
+ *
+ * \param lhs Pointers to the char array to compare
+ * \param rhs Pointers to the char array to compare
+ * \param lhs_count Length of the char array to compare
+ * \param rhs_count Length of the char array to compare
+ * \return int zero if both char sequences compare equal. negative if this
+ * appear before other, positive otherwise.
+ */
+ static int memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) : data_container{other} {}
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other) {
+ String replace{std::move(other)};
+ data_.swap(replace.data_);
+ return Downcast<String>(*this);
+}
+
+inline int String::memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count) {
+ for (size_t i = 0; i < lhs_count && i < rhs_count; ++i) {
+ if (lhs[i] < rhs[i]) {
Review comment:
Can be merged to a single liner
```if (lhs[i] < rhs[i]) return -1;```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374447787
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const { return get()->data; }
+
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
Review comment:
Style consistency, for short functions like this, we can put it directly inside the function body
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r373979652
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
Review comment:
Added `compare`. Currently adding `substr` needs to copy `StringObj` and set char array pointer accordingly, this is expensive. Otherwise we can add string view like capability to `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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800855
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) { data_container.swap(other); }
Review comment:
Use copy constructor directly in a constructor.
```
explicit FromStd(std::string other)
: data_container(other)
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376802267
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
Review comment:
Using this->compare to add more test coverage
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386077490
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,277 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return this->compare(other) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return compare(other) == 0; }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The String to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The string to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this to other
+ *
+ * \param other The character array to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other) const {
+ if (other == data()) {
+ return 0;
+ }
+ return memncmp(data(), other, size(), std::strlen(other));
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Compare two char sequence
+ *
+ * \param lhs Pointers to the char array to compare
+ * \param rhs Pointers to the char array to compare
+ * \param lhs_count Length of the char array to compare
+ * \param rhs_count Length of the char array to compare
+ * \return int zero if both char sequences compare equal. negative if this
+ * appear before other, positive otherwise.
+ */
+ static int memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) : data_container{other} {}
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other) {
+ String replace{std::move(other)};
+ data_.swap(replace.data_);
+ return Downcast<String>(*this);
+}
+
+inline int String::memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count) {
+ for (size_t i = 0; i < lhs_count && i < rhs_count; ++i) {
+ if (lhs[i] < rhs[i]) {
+ return -1;
+ }
+ if (lhs[i] > rhs[i]) {
+ return 1;
+ }
+ }
+ return lhs_count == rhs_count ? 0 : -1;
Review comment:
there are still three cases here(instead of two), depending on whether lhs_count or lhs_count is bigger.
Please add regression case to cover all the possible cases in the compare
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-580904651
gentle ping
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r384610283
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
Review comment:
I think we need more thoughts into this. I see a few problems:
-The interface of compare is not consistent with std, and we should change it to make sure it is consistent http://www.cplusplus.com/reference/string/string/compare/
- While the current usage of compare (internally) in operator== and operator!= compares the length first. We cannot guarantee that the user will have the same behavior. Given that this is a public function, we should make sure consistent behavior with std. Which leads to the conclusion that perhaps we need an explicit for loop, some testcases are desirable
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365573124
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = other;
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const { return operator->()->size; }
Review comment:
> @FrozenGene What's the behavior of `TVM_ATTRIBUTE_ALWAYS_INLINE ` in your mind?
I meant `TVM_ATTRIBUTE_ALWAYS_INLINE` is `__attribute__((always_inline))` for GCC / Clang, is `__forceinline` for MSVC. But I agree use `inline` should be enough most of 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374420184
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "vm.String";
Review comment:
done
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365549592
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = other;
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const { return operator->()->size; }
Review comment:
@FrozenGene What's the behavior of `TVM_ATTRIBUTE_ALWAYS_INLINE ` in your mind?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800566
##########
File path: include/tvm/runtime/container.h
##########
@@ -24,11 +24,35 @@
#ifndef TVM_RUNTIME_CONTAINER_H_
#define TVM_RUNTIME_CONTAINER_H_
+// Reference for feature test macros of string_view:
+// https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations
Review comment:
Let us comment about the rationale here about why do we include std::experimental::string_view in c++14. And comment that the usage is only limited to hash
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r381955560
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
Review comment:
@tqchen this is OK. Since right now `compare` expect a parameter `len` to specify the substring to compare. Whenever we call `compare`, we compare string size first.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386799867
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,286 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t> m;
Review comment:
std::unordered_map
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-593235220
@FrozenGene @icemelon9 please help to take another look. The standard lib is the core part of the system so we need extra caution in terms of reviewing. Thanks @wweic for pushing it through.
@zhiics @Hzfengsy @jroesch @ZihengJiang @yzhliu please also help to take a look if you have 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-588394175
ping @wweic
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374448894
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
Review comment:
We could use more documents here, perhaps add a few common examples.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386238682
##########
File path: python/tvm/relay/prelude.py
##########
@@ -26,6 +26,42 @@
from . import op
+class StaticTensorArrayOps(object):
Review comment:
It's a wrong commit. Just cleaned up the branch.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363280835
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = other;
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const { return operator->()->size; }
Review comment:
Maybe we should create `TVM_ATTRIBUTE_ALWAYS_INLINE` and add it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365605934
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
Review comment:
We cannot use strncmp(depending on whether its semantics is null cahr. consider use other.compare with size as in https://en.cppreference.com/w/cpp/string/basic_string/compare
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365605836
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
Review comment:
Please add hash function.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-597919820
Thanks @wweic for all the patience to polish this core data structure.
Thanks @FrozenGene @zhiics @icemelon9 for reviews.
This PR is now merged. Perhaps we can move on to add python side wrappers to the String container. And followup with the final container -- Array.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-588986264
@FrozenGene I'll do compiler test and update comment soon.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554789
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
Review comment:
Need _type_key etc.
Regression coverage:
- Add testcase that assigns an String to ObjectRef, then downcast back to 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554834
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return operator->()->size != std::strlen(other) ||
+ std::strncmp(operator->()->data, other, operator->()->size) != 0;
+}
+
+inline const char* String::c_str() const { return operator->()->data; }
+
+inline size_t String::size() const { return operator->()->size; }
Review comment:
Discuss: perhaps we should check if data is nullptr, return 0, or simply throw by CHECK(get() != nullptr);
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554667
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
Review comment:
Add function length() as well in std::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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363279604
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
Review comment:
I think we should make this be `const char* data`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-584734563
@wweic please update as per comments :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386174484
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,277 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return this->compare(other) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return compare(other) == 0; }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The String to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The string to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this to other
+ *
+ * \param other The character array to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other) const {
+ if (other == data()) {
+ return 0;
+ }
+ return memncmp(data(), other, size(), std::strlen(other));
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Compare two char sequence
+ *
+ * \param lhs Pointers to the char array to compare
+ * \param rhs Pointers to the char array to compare
+ * \param lhs_count Length of the char array to compare
+ * \param rhs_count Length of the char array to compare
+ * \return int zero if both char sequences compare equal. negative if this
+ * appear before other, positive otherwise.
+ */
+ static int memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) : data_container{other} {}
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other) {
+ String replace{std::move(other)};
+ data_.swap(replace.data_);
+ return Downcast<String>(*this);
+}
+
+inline int String::memncmp(const char* lhs, const char* rhs, size_t lhs_count,
+ size_t rhs_count) {
+ for (size_t i = 0; i < lhs_count && i < rhs_count; ++i) {
+ if (lhs[i] < rhs[i]) {
+ return -1;
+ }
+ if (lhs[i] > rhs[i]) {
+ return 1;
+ }
+ }
+ return lhs_count == rhs_count ? 0 : -1;
Review comment:
Good catch! Have added test cases.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583765522
@wweic here is an code snippet that we can reuse for hash. We should consider migrate to require c++14, which will give us string view support
```
#define TVM_USE_CXX14_STRING_VIEW \
defined(__cpp_lib_experimental_string_view) && __cpp_lib_experimental_string_view >= 201411
#define TVM_USE_CXX17_STRING_VIEW \
defined(__cpp_lib_string_view) && __cpp_lib_string_view >= 201606
#include <string>
#include <dmlc/logging.h>
#if TVM_USE_CXX17_STRING_VIEW
#include <string_view>
#elif TVM_USE_CXX14_STRING_VIEW
#include <experimental/string_view>
#endif
int main(int argc, char* argv[]) {
std::string xyz = "xyz";
#if TVM_USE_CXX17_STRING_VIEW
LOG(INFO) << "C++17=" << std::hash<std::string_view>()(xyz);
#elif TVM_USE_CXX14_STRING_VIEW
LOG(INFO) << "C++14=" << std::hash<std::experimental::string_view>()(xyz);
#else
LOG(INFO) << "C++11=" << std::hash<std::string>()(xyz);
#endif
return 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801684
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
Review comment:
document all the parameters via \param
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376696059
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
Review comment:
Thanks. done
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene edited a comment on issue #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
FrozenGene edited a comment on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-591453421
@wweic mac is libcxx. According to the doc : https://libcxx.llvm.org/docs/FeatureTestMacroTable.html it should have __cpp_lib_string_view in C++17. We should make sure libcxx on C++17 could work. C++14 doesn’t have macro to guarantee
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r384610283
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
Review comment:
I think we need more thoughts into this. I see a few problems:
- The interface of compare is not consistent with std, and we should change it to make sure it is consistent http://www.cplusplus.com/reference/string/string/compare/
- While the current usage of compare (internally) in operator== and operator!= compares the length first. We cannot guarantee that the user will have the same behavior.
- Given that this is a public function, we should make sure consistent behavior with std. Which leads to the conclusion that perhaps we need an explicit for loop, some testcases are desirable
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374448029
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const { return get()->data; }
Review comment:
remove inline for functions that is implemented inline.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386217595
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,279 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
Review comment:
no inline
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374449514
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const { return get()->data; }
+
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+}
+
} // namespace runtime
} // namespace tvm
+namespace std {
+
+template <>
+struct hash<::tvm::runtime::String> {
+ std::size_t operator()(const ::tvm::runtime::String& str) const {
+ return std::hash<std::string>()((std::string)str);
Review comment:
This is quite expensive as hash need to copy the content. Given that hash of ```std::string_view``` is not implemented until c++17, we might need to role out our own hash function
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r387973867
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,286 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered_map<String, int32_t> m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return this->compare(other) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return compare(other) == 0; }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The String to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The string to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this to other
+ *
+ * \param other The character array to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other) const {
+ if (other == data()) {
Review comment:
This seems to be not a good idea, in case other and data have different length but points to the same data content
- We need to have a regression test to cover this case
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-594367835
CI has passed after retry 2 times.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365605991
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
+}
+
+inline bool String::operator==(const char* other) const {
Review comment:
Let us add a hash implementation. perhaps as the c++ hasher convention
```c++
struct StringHash() {
size_t operator()(const String&) const;
};
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386213994
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,279 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return this->compare(other) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return compare(other) == 0; }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The String to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The string to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this to other
+ *
+ * \param other The character array to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other) const {
+ if (other == data()) {
+ return 0;
+ }
+ return memncmp(data(), other, size(), std::strlen(other));
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
Review comment:
add a `bool empty() const { return size() == 0; }` member?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801874
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
Review comment:
Let us implement operator==(and redirect !=) to be consistent with std::string version.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554682
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return operator->()->size != std::strlen(other) ||
+ std::strncmp(operator->()->data, other, operator->()->size) != 0;
+}
+
+inline const char* String::c_str() const { return operator->()->data; }
Review comment:
given these implementations are one-liner, we can consider move them iplace
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374449717
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const { return get()->data; }
+
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
Review comment:
implement a copy constructor for FromStd
```
auto ptr = std::make_object<StringObj::FromStd>(std::move(other));
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554714
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return operator->()->size != std::strlen(other) ||
+ std::strncmp(operator->()->data, other, operator->()->size) != 0;
+}
+
+inline const char* String::c_str() const { return operator->()->data; }
+
+inline size_t String::size() const { return operator->()->size; }
+
+inline const char* String::data() const { return operator->()->data; }
+
+inline String::operator std::string() const {
+ return std::string{operator->()->data, operator->()->size};
Review comment:
consider add a get() function so we can do get()->data
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365554772
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
Review comment:
add compare and substr as in https://en.cppreference.com/w/cpp/string/basic_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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800653
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) { data_container.swap(other); }
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other) {
+ String replace{std::move(other)};
+ data_.swap(replace.data_);
+ return Downcast<String>(*this);
+}
+
} // namespace runtime
} // namespace tvm
+namespace std {
+
+template <>
+struct hash<::tvm::runtime::String> {
+ std::size_t operator()(const ::tvm::runtime::String& str) const {
+#if TVM_USE_CXX17_STRING_VIEW
+ return std::hash<std::string_view>{}(
+ std::string_view{str.data(), str.size()});
+#elif TVM_USE_CXX14_STRING_VIEW
+ return std::hash<std::experimental::string_view>{}(
+ std::experimental::string_view{str.data(), str.size()});
+#else
+ return std::hash<std::string>()((std::string)str);
Review comment:
We should avoid c style casting (T)value. instead, use str.operator std::string() here
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-597579324
@tqchen sorry for the delay. Was adjusting working from china schedules. I have addressed your comments.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386238607
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,279 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return this->compare(other) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return compare(other) == 0; }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The String to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this String object to other
+ *
+ * \param other The string to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other) const {
+ return memncmp(data(), other.data(), size(), other.size());
+ }
+
+ /*!
+ * \brief Compares this to other
+ *
+ * \param other The character array to compare with.
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other) const {
+ if (other == data()) {
+ return 0;
+ }
+ return memncmp(data(), other, size(), std::strlen(other));
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
Review comment:
done
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363405556
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
Review comment:
Let us move fromStd to "implementation section in the end", as they are not supposed to be seen by the user
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374450097
##########
File path: tests/cpp/container_test.cc
##########
@@ -226,6 +226,63 @@ TEST(Map, Iterator) {
CHECK(map2[a].as<IntImmNode>()->value == 2);
}
+TEST(String, MoveFromStd) {
+ using namespace std;
+ std::string source = "this is a string";
+ std::string expect = source;
+ String s(std::move(source));
+ std::string copy = (std::string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), 0);
+}
+
+TEST(String, CopyFromStd) {
+ using namespace std;
+ std::string source = "this is a string";
+ std::string expect = source;
+ String s{source};
+ std::string copy = (std::string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), expect.size());
+}
+
+TEST(String, Comparisons) {
+ using namespace std;
+ std::string source = "a string";
+ std::string mismatch = "a string but longer";
+ String s{source};
+
+ CHECK_EQ(s == source, true);
+ CHECK_EQ(s == mismatch, false);
+ CHECK_EQ(s == source.data(), true);
+ CHECK_EQ(s == mismatch.data(), false);
+}
+
+TEST(String, c_str) {
+ using namespace std;
+ std::string source = "this is a string";
+ std::string mismatch = "mismatch";
+ String s{source};
+
+ CHECK_EQ(std::strcmp(s.c_str(), source.data()), 0);
+ CHECK_NE(std::strcmp(s.c_str(), mismatch.data()), 0);
+}
+
+TEST(String, hash) {
+ using namespace std;
+ std::string source = "this is a string";
+ String s{source};
+ std::hash<String>()(s);
Review comment:
add testcase of std::unordered_map<String, xyz>
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583793371
Thanks @tqchen! I have incorporated the snippet, please let me if I should put the macro in other places.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r373976757
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return operator->()->size != std::strlen(other) ||
+ std::strncmp(operator->()->data, other, operator->()->size) != 0;
Review comment:
we need to compare size for cases that `this` is a prefix of `other`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365592496
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The source string object
+ */
+ explicit String(const std::string& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = other;
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
+ data_ = std::move(ptr);
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const { return operator->()->size; }
Review comment:
@FrozenGene I see. Thanks for suggestion!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r365584357
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +276,137 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator==(std::string other) const;
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ inline bool operator!=(std::string other) const;
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const;
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const;
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ inline const char* data() const;
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ inline operator std::string() const;
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container.swap(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline bool String::operator==(std::string other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(std::string other) const {
+ return operator->()->size != other.size() ||
+ std::strncmp(operator->()->data, other.data(), other.size()) != 0;
+}
+
+inline bool String::operator==(const char* other) const {
+ return !operator!=(other);
+}
+
+inline bool String::operator!=(const char* other) const {
+ return operator->()->size != std::strlen(other) ||
+ std::strncmp(operator->()->data, other, operator->()->size) != 0;
+}
+
+inline const char* String::c_str() const { return operator->()->data; }
+
+inline size_t String::size() const { return operator->()->size; }
Review comment:
This reminds me that we should be better add `noexcept` keyword. if we decide to take `if data is nullptr, return 0`. As core basic data structure, we should require it more strictly.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r384610283
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
Review comment:
I think we need more thoughts into this. I see a few problems:
-The interface of compare is not consistent with std, and we should change it to make sure it is consistent http://www.cplusplus.com/reference/string/string/compare/
- While the current usage of compare (internally) in operator== and operator!= compares the length first. We cannot guarantee that the user will have the same behavior.
- Given that this is a public function, we should make sure consistent behavior with std. Which leads to the conclusion that perhaps we need an explicit for loop, some testcases are desirable
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-590978688
please look into the compilation error.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583809422
IMO, it is bad idea to introduce std::experimental to this core part. As C++ standard says: The behavior of a C++ program is undefined if it adds declarations or definitions to namespace std or to a namespace within namespace std unless otherwise specified. That is to say std::experimental’s behavior is not guaranteed. Different version of compilers / compared with C++17 std, std::experimental maybe have different result too. I strongly suggest we remove std::experimental from this pr. Or we could consider implementing our string::view if we think it is very important.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800691
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+ const auto* ptr = get();
+ if (ptr == nullptr) {
+ return 0;
+ }
+ return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) { data_container.swap(other); }
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object<StringObj::FromStd>(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other) {
+ String replace{std::move(other)};
+ data_.swap(replace.data_);
+ return Downcast<String>(*this);
+}
+
} // namespace runtime
} // namespace tvm
+namespace std {
+
+template <>
+struct hash<::tvm::runtime::String> {
+ std::size_t operator()(const ::tvm::runtime::String& str) const {
+#if TVM_USE_CXX17_STRING_VIEW
Review comment:
Add comment:
// This code falls back to a copy c++11 compilers and is recommended to be compiled with c++14
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
wweic commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376758283
##########
File path: include/tvm/runtime/container.h
##########
@@ -24,6 +24,19 @@
#ifndef TVM_RUNTIME_CONTAINER_H_
#define TVM_RUNTIME_CONTAINER_H_
+#if defined(__cpp_lib_experimental_string_view) && \
+ __cpp_lib_experimental_string_view >= 201411
+#define TVM_USE_CXX14_STRING_VIEW
+#elif defined(__cpp_lib_string_view) && __cpp_lib_string_view >= 201606
+#define TVM_USE_CXX17_STRING_VIEW
Review comment:
Good point! I have improved it to ensure both macros are defined. The old way will cause build warning `Macro expansion producing 'defined' has undefined behavior`,
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374450303
##########
File path: tests/cpp/container_test.cc
##########
@@ -226,6 +226,63 @@ TEST(Map, Iterator) {
CHECK(map2[a].as<IntImmNode>()->value == 2);
}
+TEST(String, MoveFromStd) {
+ using namespace std;
+ std::string source = "this is a string";
+ std::string expect = source;
+ String s(std::move(source));
+ std::string copy = (std::string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), 0);
+}
+
+TEST(String, CopyFromStd) {
+ using namespace std;
+ std::string source = "this is a string";
+ std::string expect = source;
+ String s{source};
Review comment:
Add testcase of normal assignment cases.
```c++
String s = "this is a string";
String s = std_string;
String s = std::move(std_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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request
#4628: [Object] Add String container
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363279814
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved std::string object
+ */
+ explicit String(std::string&& other) {
+ auto ptr = make_object<StringObj::FromStd>();
+ ptr->data_container = std::move(other);
+ ptr->size = ptr->data_container.size();
+ ptr->data = const_cast<char*>(ptr->data_container.data());
Review comment:
If we set the data be `const char*`, no need to const_cast.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add
String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583871162
@FrozenGene In general I agree that we should avoid std::experimental.
In this particular case, i think the usage is fair, because it is guarded by marco tests and is only under a very limited case we a std::hash function that can hash a string without copying it.
- T0: We could have implemented a hash function by ourselves, but the hash itself may be inconsistent with the std version.
- T1: While the std::experimental::string_view's hash could have been inconsistent with the std::string version as per compiler version(because of experimental), in practice it is consistent with std::string as per string_view proposal(and can be confirmed using different compilers). More importantly, it is also fine if the hash is inconsistent with the std ver(then we will be the case of T1.
Given the above consideration, I think it is fine to permit the limited usecase. However, I agree that we should have a more careful documentation about the std::experimental use case here and only limit it to the specific usecase.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r363405612
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,6 +275,99 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ char* data;
+
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
Review comment:
consider make it private
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-590905104
@tqchen @FrozenGene Just to give a quick update, I'm testing with clang 9.0.1 in Mac OS X. These test macros(`__cpp_lib_experimental_string_view`, and `__cpp_lib_string_view`) don't seem to be available.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] wweic commented on issue #4628: [Object] Add String
container
Posted by GitBox <gi...@apache.org>.
wweic commented on issue #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-580966470
@tqchen I'll send new revision soon.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376802203
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
+ * m[ref] = 1;
+ *
+ * // You can compare the reference object with other string objects
+ * assert(ref == "hello world", true);
+ *
+ * // You can convert the reference to std::string again
+ * string s2 = (string)ref;
+ *
+ * \endcode
+ */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Change the value the reference object points to.
+ *
+ * \param other The value for the new String
+ *
+ */
+ inline String operator=(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+ return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ if (other == get()->data) {
+ return 0;
+ }
+ return std::memcmp(get()->data, other, len);
Review comment:
Sorry, my previous comment was not correct, as memcmp did not support the case where the size of two strings did not equal each other. We will have to implement a internal memncmp using for loop(which have size of two strings here)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374448735
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
+ /*!
+ * \brief Compare is equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator==(const std::string& other) const {
+ return size() == other.size() &&
+ other.compare(0, other.size(), get()->data, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other); }
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator==(const char* other) const;
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ */
+ inline bool operator!=(const char* other) const;
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+ return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+ return std::strncmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ inline const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ inline size_t size() const {
Review comment:
remove inline
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r374450428
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +276,181 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*! \brief reference to string objects. */
+class String : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a new String object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's rvalue,
+ * it will be moved into other.
+ */
+ inline explicit String(std::string other);
+
Review comment:
Need also to overload operator=(std::string other)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #4628:
[Object] Add String container
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #4628: [Object] Add String container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r386210866
##########
File path: include/tvm/runtime/container.h
##########
@@ -274,7 +304,279 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ /*! \brief The length of the string object. */
+ uint64_t size;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordered<String, int32_t) m;
Review comment:
std::unordered_map<String, int32_t> m
----------------------------------------------------------------
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
With regards,
Apache Git Services