-
Notifications
You must be signed in to change notification settings - Fork 12.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libc++][test] Fixes constexpr char_traits. #90981
[libc++][test] Fixes constexpr char_traits. #90981
Conversation
assign(*--s1, *--s2); | ||
} | ||
return r; | ||
TEST_CONSTEXPR_CXX14 CharT* constexpr_char_traits<CharT>::move(char_type* s1, const char_type* s2, std::size_t n) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also add constexpr_char_traits
to the PR title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like short titles, so I made the title more generic and updated the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ some comments.
This was discovered by @StephanTLavavej who provided the solution they use in MSVC STL. This solution is based on that example. The same issue affects the constexpr_char_traits which was discovered in llvm#88389. This uses the same fix. Fixes: llvm#74221
b791541
to
da2f1ed
Compare
@llvm/pr-subscribers-libcxx Author: Mark de Wever (mordante) ChangesThis was discovered by @StephanTLavavej who provided the solution they use in MSVC STL. This solution is based on that example. The same issue affects the constexpr_char_traits which was discovered in #88389. This uses the same fix. Fixes: #74221 Full diff: https://github.com/llvm/llvm-project/pull/90981.diff 2 Files Affected:
diff --git a/libcxx/test/support/constexpr_char_traits.h b/libcxx/test/support/constexpr_char_traits.h
index 75380d5a7ffbb..7c487c504af13 100644
--- a/libcxx/test/support/constexpr_char_traits.h
+++ b/libcxx/test/support/constexpr_char_traits.h
@@ -16,6 +16,31 @@
#include "test_macros.h"
+// Tests whether the pointer p is in the range [first, last).
+//
+// Precondition: The range [first, last) is a valid range.
+//
+// Typically the pointers are compared with less than. This is not allowed when
+// the pointers belong to different ranges, which is UB. Typically, this is
+// benign at run-time, however since UB is not allowed during constant
+// evaluation this does not compile. This function does the validation without
+// UB.
+//
+// When p is in the range [first, last) the data can be copied from the
+// beginning to the end. Otherwise it needs to be copied from the end to the
+// beginning.
+template <class CharT>
+TEST_CONSTEXPR_CXX14 bool is_pointer_in_range(const CharT* first, const CharT* last, const CharT* p) {
+ if (first == p) // Needed when n == 0
+ return true;
+
+ for (; first != last; ++first)
+ if (first == p)
+ return true;
+
+ return false;
+}
+
template <class CharT>
struct constexpr_char_traits
{
@@ -98,23 +123,21 @@ constexpr_char_traits<CharT>::find(const char_type* s, std::size_t n, const char
}
template <class CharT>
-TEST_CONSTEXPR_CXX14 CharT*
-constexpr_char_traits<CharT>::move(char_type* s1, const char_type* s2, std::size_t n)
-{
- char_type* r = s1;
- if (s1 < s2)
- {
- for (; n; --n, ++s1, ++s2)
- assign(*s1, *s2);
- }
- else if (s2 < s1)
- {
- s1 += n;
- s2 += n;
- for (; n; --n)
- assign(*--s1, *--s2);
- }
- return r;
+TEST_CONSTEXPR_CXX14 CharT* constexpr_char_traits<CharT>::move(char_type* s1, const char_type* s2, std::size_t n) {
+ if (s1 == s2)
+ return s1;
+
+ char_type* r = s1;
+ if (is_pointer_in_range(s1, s1 + n, s2)) {
+ for (; n; --n)
+ assign(*s1++, *s2++);
+ } else {
+ s1 += n;
+ s2 += n;
+ for (; n; --n)
+ assign(*--s1, *--s2);
+ }
+ return r;
}
template <class CharT>
diff --git a/libcxx/test/support/nasty_string.h b/libcxx/test/support/nasty_string.h
index 672c3cb4ed9ea..ea9d83ccf282a 100644
--- a/libcxx/test/support/nasty_string.h
+++ b/libcxx/test/support/nasty_string.h
@@ -16,6 +16,7 @@
#include "make_string.h"
#include "test_macros.h"
+#include "constexpr_char_traits.h" // is_pointer_in_range
// This defines a nasty_string similar to nasty_containers. This string's
// value_type does operator hijacking, which allows us to ensure that the
@@ -118,11 +119,14 @@ constexpr const nasty_char* nasty_char_traits::find(const nasty_char* s, std::si
}
constexpr nasty_char* nasty_char_traits::move(nasty_char* s1, const nasty_char* s2, std::size_t n) {
+ if (s1 == s2)
+ return s1;
+
nasty_char* r = s1;
- if (s1 < s2) {
- for (; n; --n, ++s1, ++s2)
- assign(*s1, *s2);
- } else if (s2 < s1) {
+ if (is_pointer_in_range(s1, s1 + n, s2)) {
+ for (; n; --n)
+ assign(*s1++, *s2++);
+ } else {
s1 += n;
s2 += n;
for (; n; --n)
|
The issue in nasty_char_traits was discovered by @StephanTLavavej who provided
the solution they use in MSVC STL. This solution is based on that example.
The same issue affects the constexpr_char_traits which was discovered in
#88389. This uses the same fix.
Fixes: #74221