[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
BillyONeal added a comment. Tim Song suggests that http://eel.is/c++draft/string.require#2 indicates that basic_string actually does need to provide the strong guarantee here. While I think that wording is a mess I think I'll fix our basic_string to do that instead of that part of this change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60023/new/ https://reviews.llvm.org/D60023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
BillyONeal updated this revision to Diff 193216.
BillyONeal added a comment.
Fixed misspelled test macro.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60023/new/
https://reviews.llvm.org/D60023
Files:
test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
#include "min_allocator.h"
#ifndef TEST_HAS_NO_EXCEPTIONS
-template
struct alloc_imp {
bool active;
alloc_imp() : active(true) {}
+template
T* allocate(std::size_t n)
{
if (active)
@@ -32,6 +32,7 @@
throw std::bad_alloc();
}
+template
void deallocate(T* p, std::size_t) { std::free(p); }
void activate () { active = true; }
void deactivate() { active = false; }
@@ -42,14 +43,14 @@
typedef T value_type;
typedef std::true_type propagate_on_container_copy_assignment;
-alloc_imp *imp;
+alloc_imp *imp;
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
template
poca_alloc(const poca_alloc& other) : imp(other.imp) {}
-T* allocate (std::size_t n) { return imp->allocate(n);}
+T* allocate (std::size_t n) { return imp->allocate(n);}
void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
};
@@ -112,8 +113,8 @@
const char * p1 = "This is my first string";
const char * p2 = "This is my second string";
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
S s1(p1, A());
S s2(p2, A());
@@ -122,7 +123,11 @@
imp2.deactivate();
test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy
assignment operator,
+// but the standard only requires the basic guarantee:
+LIBCPP_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string
object.
+assert(s1.empty());
assert(s2 == p2);
}
#endif
Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
#include "min_allocator.h"
#ifndef TEST_HAS_NO_EXCEPTIONS
-template
struct alloc_imp {
bool active;
alloc_imp() : active(true) {}
+template
T* allocate(std::size_t n)
{
if (active)
@@ -32,6 +32,7 @@
throw std::bad_alloc();
}
+template
void deallocate(T* p, std::size_t) { std::free(p); }
void activate () { active = true; }
void deactivate() { active = false; }
@@ -42,14 +43,14 @@
typedef T value_type;
typedef std::true_type propagate_on_container_copy_assignment;
-alloc_imp *imp;
+alloc_imp *imp;
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
template
poca_alloc(const poca_alloc& other) : imp(other.imp) {}
-T* allocate (std::size_t n) { return imp->allocate(n);}
+T* allocate (std::size_t n) { return imp->allocate(n);}
void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
};
@@ -112,8 +113,8 @@
const char * p1 = "This is my first string";
const char * p2 = "This is my second string";
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
S s1(p1, A());
S s2(p2, A());
@@ -122,7 +123,11 @@
imp2.deactivate();
test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy assignment operator,
+// but the standard only requires the basic guarantee:
+LIBCPP_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string object.
+assert(s1.empty());
assert(s2 == p2);
}
#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
CaseyCarter requested changes to this revision. CaseyCarter added inline comments. This revision now requires changes to proceed. Comment at: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:128 +// but the standard only requires the basic guarantee: +_LIBCXX_ASSERT(s1 == p1); +s1.clear(); // under the basic guarantee, s1 must still be a valid string object. `LIBCPP_ASSERT(s1 == p1)` (NB: no leading underscore) in test code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60023/new/ https://reviews.llvm.org/D60023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
BillyONeal updated this revision to Diff 193157.
BillyONeal edited the summary of this revision.
BillyONeal added a comment.
Fix asserts for the strong EH guarantee.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60023/new/
https://reviews.llvm.org/D60023
Files:
test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
#include "min_allocator.h"
#ifndef TEST_HAS_NO_EXCEPTIONS
-template
struct alloc_imp {
bool active;
alloc_imp() : active(true) {}
+template
T* allocate(std::size_t n)
{
if (active)
@@ -32,6 +32,7 @@
throw std::bad_alloc();
}
+template
void deallocate(T* p, std::size_t) { std::free(p); }
void activate () { active = true; }
void deactivate() { active = false; }
@@ -42,14 +43,14 @@
typedef T value_type;
typedef std::true_type propagate_on_container_copy_assignment;
-alloc_imp *imp;
+alloc_imp *imp;
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
template
poca_alloc(const poca_alloc& other) : imp(other.imp) {}
-T* allocate (std::size_t n) { return imp->allocate(n);}
+T* allocate (std::size_t n) { return imp->allocate(n);}
void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
};
@@ -112,8 +113,8 @@
const char * p1 = "This is my first string";
const char * p2 = "This is my second string";
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
S s1(p1, A());
S s2(p2, A());
@@ -122,7 +123,11 @@
imp2.deactivate();
test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy
assignment operator,
+// but the standard only requires the basic guarantee:
+_LIBCXX_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string
object.
+assert(s1.empty());
assert(s2 == p2);
}
#endif
Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
#include "min_allocator.h"
#ifndef TEST_HAS_NO_EXCEPTIONS
-template
struct alloc_imp {
bool active;
alloc_imp() : active(true) {}
+template
T* allocate(std::size_t n)
{
if (active)
@@ -32,6 +32,7 @@
throw std::bad_alloc();
}
+template
void deallocate(T* p, std::size_t) { std::free(p); }
void activate () { active = true; }
void deactivate() { active = false; }
@@ -42,14 +43,14 @@
typedef T value_type;
typedef std::true_type propagate_on_container_copy_assignment;
-alloc_imp *imp;
+alloc_imp *imp;
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
template
poca_alloc(const poca_alloc& other) : imp(other.imp) {}
-T* allocate (std::size_t n) { return imp->allocate(n);}
+T* allocate (std::size_t n) { return imp->allocate(n);}
void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
};
@@ -112,8 +113,8 @@
const char * p1 = "This is my first string";
const char * p2 = "This is my second string";
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
S s1(p1, A());
S s2(p2, A());
@@ -122,7 +123,11 @@
imp2.deactivate();
test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy assignment operator,
+// but the standard only requires the basic guarantee:
+_LIBCXX_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string object.
+assert(s1.empty());
assert(s2 == p2);
}
#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
ldionne requested changes to this revision. ldionne added inline comments. This revision now requires changes to proceed. Comment at: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:125 test_assign(s1, s2); assert(s1 == p1); assert(s2 == p2); BillyONeal wrote: > This particular assert tests that the copy assignment operator provides the > strong exception safety guarantee, but the standard requires only the basic > guarantee. Should this be _LIBCXX_ASSERT? Yes, I think it should (ideally with a short comment). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60023/new/ https://reviews.llvm.org/D60023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
BillyONeal marked an inline comment as done. BillyONeal added inline comments. Comment at: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:125 test_assign(s1, s2); assert(s1 == p1); assert(s2 == p2); This particular assert tests that the copy assignment operator provides the strong exception safety guarantee, but the standard requires only the basic guarantee. Should this be _LIBCXX_ASSERT? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60023/new/ https://reviews.llvm.org/D60023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
BillyONeal marked 2 inline comments as done. BillyONeal added inline comments. Comment at: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:123 imp2.deactivate(); test_assign(s1, s2); BillyONeal wrote: > We still fail this test in debug mode because we need to allocate a new > _Container_proxy from imp2 in this assignment. Should this be libcxx_assert? Hmmm this is copy assignment, not move assignment, so we still have a bug. I should not try writing CR comments at 2 in the morning. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60023/new/ https://reviews.llvm.org/D60023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
BillyONeal added inline comments. Comment at: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:123 imp2.deactivate(); test_assign(s1, s2); We still fail this test in debug mode because we need to allocate a new _Container_proxy from imp2 in this assignment. Should this be libcxx_assert? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60023/new/ https://reviews.llvm.org/D60023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.
BillyONeal created this revision.
BillyONeal added reviewers: EricWF, ldionne, mclow.lists.
BillyONeal added a project: libc++.
Herald added subscribers: jdoerfert, dexonsmith.
copy_alloc.pass.cpp contains an allocator that tries to be rebindable; but if
it actually does that it fails because the alloc_imp type is not rebindable (so
you get failure to compile because alloc_imp* isn't convertible to
alloc_imp*).
The fix is to make the members of alloc_impl templates instead of making
alloc_imp itself a template.
This allows the test for MSVC++ because we need to rebind for _Container_proxy.
https://reviews.llvm.org/D60023
Files:
test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
#include "min_allocator.h"
#ifndef TEST_HAS_NO_EXCEPTIONS
-template
struct alloc_imp {
bool active;
alloc_imp() : active(true) {}
+template
T* allocate(std::size_t n)
{
if (active)
@@ -32,6 +32,7 @@
throw std::bad_alloc();
}
+template
void deallocate(T* p, std::size_t) { std::free(p); }
void activate () { active = true; }
void deactivate() { active = false; }
@@ -42,14 +43,14 @@
typedef T value_type;
typedef std::true_type propagate_on_container_copy_assignment;
-alloc_imp *imp;
+alloc_imp *imp;
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
template
poca_alloc(const poca_alloc& other) : imp(other.imp) {}
-T* allocate (std::size_t n) { return imp->allocate(n);}
+T* allocate (std::size_t n) { return imp->allocate(n);}
void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
};
@@ -112,8 +113,8 @@
const char * p1 = "This is my first string";
const char * p2 = "This is my second string";
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
S s1(p1, A());
S s2(p2, A());
Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
#include "min_allocator.h"
#ifndef TEST_HAS_NO_EXCEPTIONS
-template
struct alloc_imp {
bool active;
alloc_imp() : active(true) {}
+template
T* allocate(std::size_t n)
{
if (active)
@@ -32,6 +32,7 @@
throw std::bad_alloc();
}
+template
void deallocate(T* p, std::size_t) { std::free(p); }
void activate () { active = true; }
void deactivate() { active = false; }
@@ -42,14 +43,14 @@
typedef T value_type;
typedef std::true_type propagate_on_container_copy_assignment;
-alloc_imp *imp;
+alloc_imp *imp;
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
template
poca_alloc(const poca_alloc& other) : imp(other.imp) {}
-T* allocate (std::size_t n) { return imp->allocate(n);}
+T* allocate (std::size_t n) { return imp->allocate(n);}
void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
};
@@ -112,8 +113,8 @@
const char * p1 = "This is my first string";
const char * p2 = "This is my second string";
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
S s1(p1, A());
S s2(p2, A());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
