[ADT] Teach StringRef to derive from std::string_view #113752
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch makes minimum changes to have StringRef derive from
std::string_view, with an eventual goal of removing StringRef
completely. Subsequent patches will further clean up the remaining
bits.
I've chosen public inheritance over private one because our codebase
relies on implicit conversions to std::string_view, which is currently
provided by:
constexpr operator std::string_view() const {
return std::string_view(data(), size());
}
This implicit conversion stops applying once we use std::string_view
as a base class, public or private. If we chose a private base class,
we would lose the implicit conversion.
|
@llvm/pr-subscribers-llvm-adt Author: Kazu Hirata (kazutakahirata) ChangesThis patch makes minimum changes to have StringRef derive from I've chosen public inheritance over private one because our codebase constexpr operator std::string_view() const { This implicit conversion stops applying once we use std::string_view Full diff: https://github.com/llvm/llvm-project/pull/113752.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ADT/StringRef.h b/llvm/include/llvm/ADT/StringRef.h
index d5f30b88c4c6a2..49b6b8ff52abec 100644
--- a/llvm/include/llvm/ADT/StringRef.h
+++ b/llvm/include/llvm/ADT/StringRef.h
@@ -48,7 +48,9 @@ namespace llvm {
/// situations where the character data resides in some other buffer, whose
/// lifetime extends past that of the StringRef. For this reason, it is not in
/// general safe to store a StringRef.
- class LLVM_GSL_POINTER StringRef {
+ class LLVM_GSL_POINTER StringRef : public std::string_view {
+ using Base = std::string_view;
+
public:
static constexpr size_t npos = ~size_t(0);
@@ -60,12 +62,6 @@ namespace llvm {
using const_reverse_iterator = std::reverse_iterator<const_iterator>;
private:
- /// 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
static int compareMemory(const char *Lhs, const char *Rhs, size_t Length) {
@@ -86,27 +82,25 @@ namespace llvm {
/// Construct a string ref from a cstring.
/*implicit*/ constexpr StringRef(const char *Str)
- : Data(Str), Length(Str ?
+ : Base(Str, 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, size_t length)
- : Data(data), Length(length) {}
+ : Base(data, length) {}
/// Construct a string ref from an std::string.
- /*implicit*/ StringRef(const std::string &Str)
- : Data(Str.data()), Length(Str.length()) {}
+ /*implicit*/ StringRef(const std::string &Str) : Base(Str) {}
/// Construct a string ref from an std::string_view.
- /*implicit*/ constexpr StringRef(std::string_view Str)
- : Data(Str.data()), Length(Str.size()) {}
+ /*implicit*/ constexpr StringRef(std::string_view Str) : Base(Str) {}
/// @}
/// @name Iterators
@@ -138,16 +132,9 @@ namespace llvm {
/// @name String Operations
/// @{
- /// data - Get a pointer to the start of the string (which may not be null
- /// terminated).
- [[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 Length; }
-
/// front - Get the first character in the string.
[[nodiscard]] char front() const {
assert(!empty());
@@ -248,14 +235,6 @@ namespace llvm {
std::enable_if_t<std::is_same<T, std::string>::value, StringRef> &
operator=(T &&Str) = delete;
- /// @}
- /// @name Type Conversions
- /// @{
-
- constexpr operator std::string_view() const {
- return std::string_view(data(), size());
- }
-
/// @}
/// @name String Predicates
/// @{
|
MaskRay
approved these changes
Oct 26, 2024
|
Nice. Do you plan a brief announcement on discourse for the plan? |
Good idea! https://discourse.llvm.org/t/migrating-llvm-stringref-to-std-string-view/82785 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/7810 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/8952 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/7485 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/10163 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/105/builds/3373 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/28/builds/3692 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/9528 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/1065 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/158/builds/3263 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/7316 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/6715 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/8179 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/7318 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/5751 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/3919 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/7405 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/8/builds/6267 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/131/builds/9115 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7602 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/7/builds/6300 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/212 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/130/builds/5375 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/5461 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/4817 Here is the relevant piece of the build log for the reference |
MaskRay
added a commit
that referenced
this pull request
Oct 26, 2024
Reverts #113752 Many build bot failures
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/5483 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/5370 Here is the relevant piece of the build log for the reference |
kazutakahirata
added a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Oct 26, 2024
This patch makes minimum changes to replace Data and Length with an instance of std::string_view. Previously, I opted for public inheritance (llvm#113752), but I encountered a lot of errors from gcc stemming from ambiguity between std::string_view and StringRef. The composition approach in this patch gives us greater control at the expense of forwarder functions.
kazutakahirata
added a commit
that referenced
this pull request
Oct 27, 2024
This patch makes minimum changes to replace Data and Length with an instance of std::string_view. Previously, I opted for public inheritance (#113752), but I encountered a lot of errors from gcc stemming from ambiguity between std::string_view and StringRef. The composition approach in this patch gives us greater control at the expense of forwarder functions.
|
Why was this merged before the RFC was discussed (or accepted)? Edit: oh, I see, @MaskRay approved, that makes more sense. However, we should still not land these changes until the RFC is actually approved because it's not clear the changes are beneficial unless we're planning to go forward with the whole RFC. |
|
This patch was posted before https://discourse.llvm.org/t/migrating-llvm-stringref-to-std-string-view/82785 . I thought if StringRef could derive from std::string_view, it would be really nice to simplify interop with std::string_view, whether or not we decide to keep StringRef (perhaps we would only use StringRef as temporary objects for its specific APIs, instead of storing StringRef in data structures). However, there were many issues due to GCC overload resolution. As it turns out, the subclass approach doesn't work out. I do agree that the changes need more discussion. |
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
This patch makes minimum changes to have StringRef derive from
std::string_view, with an eventual goal of removing StringRef
completely. Subsequent patches will further clean up the remaining
bits.
I've chosen public inheritance over private one because our codebase
relies on implicit conversions to std::string_view, which is currently
provided by:
constexpr operator std::string_view() const {
return std::string_view(data(), size());
}
This implicit conversion stops applying once we use std::string_view
as a base class, public or private. If we chose a private base class,
we would lose the implicit conversion.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…113767) Reverts llvm#113752 Many build bot failures
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
This patch makes minimum changes to replace Data and Length with an instance of std::string_view. Previously, I opted for public inheritance (llvm#113752), but I encountered a lot of errors from gcc stemming from ambiguity between std::string_view and StringRef. The composition approach in this patch gives us greater control at the expense of forwarder functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch makes minimum changes to have StringRef derive from
std::string_view, with an eventual goal of removing StringRef
completely. Subsequent patches will further clean up the remaining
bits.
I've chosen public inheritance over private one because our codebase
relies on implicit conversions to std::string_view, which is currently
provided by:
constexpr operator std::string_view() const {
return std::string_view(data(), size());
}
This implicit conversion stops applying once we use std::string_view
as a base class, public or private. If we chose a private base class,
we would lose the implicit conversion.