Review Comments

include/tvm/runtime/container.h

@@ -282,17 +282,13 @@ class StringObj : public Object {
   uint32_t size;
 
   /*! \brief The pointer to string data. */
-  char* data;
+  const char* data;
 
+ private:
   /*! \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;
+  friend class String;
 };
 
 /*! \brief reference to string objects. */
@@ -301,73 +297,115 @@ class String : public ObjectRef {
   /*!
    * \brief Construct a new String object
    *
-   * \param other The moved std::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) {
-    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);
-  }
+  inline explicit String(std::string other);
 
   /*!
-   * \brief Construct a new String object
+   * \brief Compare is equal to other std::string
    *
-   * \param other The source string object
+   * \param other The other string
    */
-  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);
-  }
+  inline bool operator==(std::string other) const;
 
   /*!
-   * \brief Return the length of the string
+   * \brief Compare is not equal to other std::string
    *
-   * \return size_t string length
+   * \param other The other string
    */
-  size_t size() const { return operator->()->size; }
+  inline bool operator!=(std::string other) const;
 
   /*!
-   * \brief Return the data pointer
+   * \brief Compare is equal to other char string
    *
-   * \return char* data pointer
+   * \param other The other char string
    */
-  char* data() const { return operator->()->data; }
+  inline bool operator==(const char* other) const;
 
   /*!
-   * \brief Create a String reference from std::string rvalue
+   * \brief Compare is not equal to other char string
    *
-   * \param other The source std::string object.
-   * \return String The String reference.
+   * \param other The other char string
    */
-  static String FromStd(std::string&& other) {
-    return String(std::move(other));
-  }
+  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 Create a String reference from std::string reference
+   * \brief Return the data pointer
    *
-   * \param other The source std::string object.
-   * \return String The String reference.
+   * \return const char* data pointer
    */
-  static String FromStd(const std::string& other) { return String(other); }
+  inline const char* data() const;
 
   /*!
    * \brief Convert String to an std::sting object
    *
    * \return std::string
    */
-  operator std::string() const {
-    return std::string{operator->()->data, operator->()->size};
-  }
+  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};
+}
+
 }  // namespace runtime
 }  // namespace tvm
 

tests/cpp/container_test.cc

@@ -229,7 +229,7 @@ TEST(String, MoveFromStd) {
   using namespace std;
   std::string source = "this is a string";
   std::string expect = source;
-  String s = String::FromStd(std::move(source));
+  String s(std::move(source));
   std::string copy = (std::string)s;
   CHECK_EQ(copy, expect);
   CHECK_EQ(source.size(), 0);
@@ -239,12 +239,34 @@ TEST(String, CopyFromStd) {
   using namespace std;
   std::string source = "this is a string";
   std::string expect = source;
-  String s = String::FromStd(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 = "this is a string";
+  std::string mismatch = "different string";
+  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);
+}
+
 int main(int argc, char** argv) {
   testing::InitGoogleTest(&argc, argv);
   testing::FLAGS_gtest_death_test_style = "threadsafe";