Skip to content

Conversation

@kazutakahirata
Copy link
Contributor

@kazutakahirata kazutakahirata commented Oct 29, 2024

This patch reverts commit 89b5d88.

Some sanitizer failures have been reported, indicating that StringRef
and std::string_view handle data == nulptr differently. Also, they
support different values for the max size (size_t v.s. ptrdiff_t).

Thanks goes to Jorge Gorbe Moya for reporting these.

This patch reverts commit 89b5d88.

Some sanitizer failures have been reported, indivating that StringRef
and std::string_view handle data == nulptr differently.  Also, they
support different values for the max size (size_t v.s. ptrdiff_t).

Thanks goes to Jorge Gorbe Moya for reporting these.
@llvmbot
Copy link
Member

llvmbot commented Oct 29, 2024

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

This patch reverts commit 89b5d88.

Some sanitizer failures have been reported, indivating that StringRef
and std::string_view handle data == nulptr differently. Also, they
support different values for the max size (size_t v.s. ptrdiff_t).

Thanks goes to Jorge Gorbe Moya for reporting these.


Full diff: https://github.com/llvm/llvm-project/pull/114133.diff

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/StringRef.h (+16-10)
diff --git a/llvm/include/llvm/ADT/StringRef.h b/llvm/include/llvm/ADT/StringRef.h
index 0dcd4d90086eff..5b525c8e56ecc9 100644
--- a/llvm/include/llvm/ADT/StringRef.h
+++ b/llvm/include/llvm/ADT/StringRef.h
@@ -60,7 +60,11 @@ namespace llvm {
     using const_reverse_iterator = std::reverse_iterator<const_iterator>;
 
   private:
-    std::string_view View;
+    /// The start of the string, in an external buffer.
+    const char *Data = nullptr;
+
+    /// The length of the string.
+    size_t Length = 0;
 
     // Workaround memcmp issue with null pointers (undefined behavior)
     // by providing a specialized version
@@ -82,26 +86,28 @@ namespace llvm {
 
     /// Construct a string ref from a cstring.
     /*implicit*/ constexpr StringRef(const char *Str LLVM_LIFETIME_BOUND)
-        : View(Str, Str ?
+        : Data(Str), Length(Str ?
     // GCC 7 doesn't have constexpr char_traits. Fall back to __builtin_strlen.
 #if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE < 8
-                        __builtin_strlen(Str)
+                                __builtin_strlen(Str)
 #else
-                        std::char_traits<char>::length(Str)
+                                std::char_traits<char>::length(Str)
 #endif
-                        : 0) {
+                                : 0) {
     }
 
     /// Construct a string ref from a pointer and length.
     /*implicit*/ constexpr StringRef(const char *data LLVM_LIFETIME_BOUND,
                                      size_t length)
-        : View(data, length) {}
+        : Data(data), Length(length) {}
 
     /// Construct a string ref from an std::string.
-    /*implicit*/ StringRef(const std::string &Str) : View(Str) {}
+    /*implicit*/ StringRef(const std::string &Str)
+        : Data(Str.data()), Length(Str.length()) {}
 
     /// Construct a string ref from an std::string_view.
-    /*implicit*/ constexpr StringRef(std::string_view Str) : View(Str) {}
+    /*implicit*/ constexpr StringRef(std::string_view Str)
+        : Data(Str.data()), Length(Str.size()) {}
 
     /// @}
     /// @name Iterators
@@ -135,13 +141,13 @@ namespace llvm {
 
     /// data - Get a pointer to the start of the string (which may not be null
     /// terminated).
-    [[nodiscard]] constexpr const char *data() const { return View.data(); }
+    [[nodiscard]] constexpr const char *data() const { return Data; }
 
     /// empty - Check if the string is empty.
     [[nodiscard]] constexpr bool empty() const { return size() == 0; }
 
     /// size - Get the string size.
-    [[nodiscard]] constexpr size_t size() const { return View.size(); }
+    [[nodiscard]] constexpr size_t size() const { return Length; }
 
     /// front - Get the first character in the string.
     [[nodiscard]] char front() const {

Copy link
Collaborator

@slackito slackito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kazutakahirata kazutakahirata merged commit 5cfb07a into llvm:main Oct 29, 2024
7 of 9 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_001_StringRef_revert branch October 29, 2024 21:46
@kuhar
Copy link
Member

kuhar commented Oct 29, 2024

@kazutakahirata

Some sanitizer failures have been reported, indicating that StringRef
and std::string_view handle data == nulptr differently. Also, they
support different values for the max size (size_t v.s. ptrdiff_t).

We should pre-commit unit tests that cover these differences

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…lvm#114133)

This patch reverts commit 89b5d88.

Some sanitizer failures have been reported, indicating that StringRef
and std::string_view handle data == nulptr differently.  Also, they
support different values for the max size (size_t v.s. ptrdiff_t).

Thanks goes to Jorge Gorbe Moya for reporting these.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants