Don't include what you don't use. You only use
<string.h>
and<stdio.h>
.
While including<vector>
only costs compile-time, including<iostream>
potentially increases your executable-size and startup-/shutdown-time.You should avoid
using namespace std;
It's a plague and just biding its time to bite you later.
Why is "using namespace std;" considered bad practice?I'm sorry, but your comments seem to be completely useless. At least I couldn't extract any useful information from them. So, purge them.
Your
DynString
seems to be a thin wrapper around 0-terminated strings. Are you sure you will never need embedded zeroes, nor have enough use of the length that caching it might be worthwhile?
Another disadvantage is that due to the also missing capacity appending to the string will always re-allocate.Raw owning pointers are a bad idea. Not only are they leak-prone per se, things get even harder with exceptions.
Consider a move to the no-cost abstraction ofstd::unique_ptr
.Your
nullptr
-handling is inconsistent. Isnullptr
UB, an empty string, or preserved as a separate state?
Both of the latter two make sense.As an aside, prefer
nullptr
toNULL
or0
. Not only does it have a distinctive type, it also cannot be confused with an integer.Use
std::size_t
for indices and sizes. That's what it's for.You forgot to mark your functions
noexcept
where allowed.Your default-constructor should be marked
constexpr
, or allocate a 0-length string.In
DynString(const char*)
you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficientmemcpy
instead of doing a less performantstrcpy
?
At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark itexplicit
.DynString(const DynString&)
should delegate toDynString(const char*)
instead of writing everything out all over again.You are missing a move-ctor.
The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.
You are missing a constant overload for
operator []
. And anyway, the index should be astd::size_t
, not anint
.Your
operator+(DynString&)
(andoperator+(const char*)
too) is a first-grade mess:
Don't include what you don't use. You only use
<string.h>
and<stdio.h>
.
While including<vector>
only costs compile-time, including<iostream>
potentially increases your executable-size and startup-/shutdown-time.You should avoid
using namespace std;
It's a plague and just biding its time to bite you later.
Why is "using namespace std;" considered bad practice?I'm sorry, but your comments seem to be completely useless. At least I couldn't extract any useful information from them. So, purge them.
Your
DynString
seems to be a thin wrapper around 0-terminated strings. Are you sure you will never need embedded zeroes, nor have enough use of the length that caching it might be worthwhile?
Another disadvantage is that due to the also missing capacity appending to the string will always re-allocate.Raw owning pointers are a bad idea. Not only are they leak-prone per se, things get even harder with exceptions.
Consider a move to the no-cost abstraction ofstd::unique_ptr
.Your
nullptr
-handling is inconsistent. Isnullptr
UB, an empty string, or preserved as a separate state?
Both of the latter two make sense.As an aside, prefer
nullptr
toNULL
or0
. Not only does it have a distinctive type, it also cannot be confused with an integer.Use
std::size_t
for indices and sizes. That's what it's for.You forgot to mark your functions
noexcept
where allowed.Your default-constructor should be marked
constexpr
, or allocate a 0-length string.In
DynString(const char*)
you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficientmemcpy
instead of doing a less performantstrcpy
?
At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark itexplicit
.DynString(const DynString&)
should delegate toDynString(const char*)
instead of writing everything out all over again.You are missing a move-ctor.
The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.
You are missing a constant overload for
operator []
. And anyway, the index should be astd::size_t
, not anint
.Your
operator+(DynString&)
(andoperator+(const char*)
too) is a first-grade mess:
Don't include what you don't use. You only use
<string.h>
and<stdio.h>
.
While including<vector>
only costs compile-time, including<iostream>
potentially increases your executable-size and startup-/shutdown-time.You should avoid
using namespace std;
It's a plague and just biding its time to bite you later.
Why is "using namespace std;" considered bad practice?I'm sorry, but your comments seem to be completely useless. At least I couldn't extract any useful information from them. So, purge them.
Your
DynString
seems to be a thin wrapper around 0-terminated strings. Are you sure you will never need embedded zeroes, nor have enough use of the length that caching it might be worthwhile?
Another disadvantage is that due to the also missing capacity appending to the string will always re-allocate.Raw owning pointers are a bad idea. Not only are they leak-prone per se, things get even harder with exceptions.
Consider a move to the no-cost abstraction ofstd::unique_ptr
.Your
nullptr
-handling is inconsistent. Isnullptr
UB, an empty string, or preserved as a separate state?As an aside, prefer
nullptr
toNULL
or0
. Not only does it have a distinctive type, it also cannot be confused with an integer.Use
std::size_t
for indices and sizes. That's what it's for.You forgot to mark your functions
noexcept
where allowed.Your default-constructor should be marked
constexpr
, or allocate a 0-length string.In
DynString(const char*)
you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficientmemcpy
instead of doing a less performantstrcpy
?
At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark itexplicit
.DynString(const DynString&)
should delegate toDynString(const char*)
instead of writing everything out all over again.You are missing a move-ctor.
The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.
You are missing a constant overload for
operator []
. And anyway, the index should be astd::size_t
, not anint
.Your
operator+(DynString&)
(andoperator+(const char*)
too) is a first-grade mess:
#include <cstring>
#include <algorithm>
#include <utility>
#include <iterator>
#include <vector>
class DynString {
char *c;
DynString(std::picewise_construct_t, const char* a, const char* b) : c() {
auto na = std::strlen(a), nb = std::strlen(b);
c = new char[a + b + 1 > a ? a + b + 1 : SIZE_MAX];
std::copy_n(a, na, c);
std::copy_n(b, nb + 1, c + na);
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::forward_iterator_tag, It first, It last) : c() {
auto n = std::distance(first, last);
c = new char[0 <= n && n < SIZE_MAX ? std::size_t(n) + 1 : SIZE_MAX];
try {
std::copy_n(first, std::size_t(n), c);
c[n] = 0;
} catch(...) {
delete [] c;
throw;
}
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::input_iterator_tag, It first, It last) : c() {
std::vector<char> v(first, last);
swap(DynString({v.begin(), v.end())});
}
public:
DynString() : c(new char[1]{}) {}
DynString(const char* s) : DynString(s, s + std::strlen(s)) {}
DynString(const DynString& s) : DynString(s.c) {}
DynString(DynString&& s) : DynString() { swap(s); }
template<class It, decltype((void)DynString(std::iterator_traits<It>::iterator_category(), first, last))...>
DynString(It first, It last) : DynString(std::iterator_traits<It>::iterator_category(), first, last) {}
~DynString() { delete[] c; }
friend ostream& operator<<(ostream& lhs, const DynString& a) { return lhs << a.c; }
char& operator[](std::size_t i) noexcept { return c[i]; }
const char& operator[](std::size_t i) const noexcept { return c[i]; }
auto data() noexcept { return c; }
auto data() const noexcept { return c; }
auto begin() noexcept { return c; }
auto begin() const noexcept { return c; }
auto end() noexcept { return c + size(); }
auto end() const noexcept { return c + size(); }
auto rbegin() noexcept { return std::make_reverse_iterator(end()); }
auto rbegin() const noexcept { return std::make_reverse_iterator(end()); }
auto rend() noexcept { return std::make_reverse_iterator(begin()); }
auto rend() const noexcept { return std::make_reverse_iterator(begin()); }
auto size() const noexcept { return std::strlen(c); }
friend DynString operator+(const DynString& a, const DynString& b)
{ return {std::piecewise_construct, a.c, b.c}; }
friend DynString operator+(const char* a, const DynString& b)
{ return {std::piecewise_construct, a, b.c}; }
friend DynString operator+(const DynString& a, const char* b)
{ return {std::piecewise_construct, a.c, b}; }
DynString& operator+=(const DynString& s) { return *this += s.c; }
DynString& operator+=(const char* s) { return *this = *this + s; }
DynString& operator=(DynString s) { swap(s); return *this; }
void swap(DynString& o) noexcept { using std::swap; swap(c, o.c); }
};
#include <cstring>
#include <algorithm>
#include <utility>
#include <iterator>
#include <vector>
class DynString {
char *c;
DynString(std::picewise_construct_t, const char* a, const char* b) {
auto na = std::strlen(a), nb = std::strlen(b);
c = new char[a + b + 1 > a ? a + b + 1 : SIZE_MAX];
std::copy_n(a, na, c);
std::copy_n(b, nb + 1, c + na);
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::forward_iterator_tag, It first, It last) {
auto n = std::distance(first, last);
c = new char[0 <= n && n < SIZE_MAX ? std::size_t(n) + 1 : SIZE_MAX];
try {
std::copy_n(first, std::size_t(n), c);
c[n] = 0;
} catch(...) {
delete [] c;
throw;
}
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::input_iterator_tag, It first, It last) : c() {
std::vector<char> v(first, last);
swap(DynString(v.begin(), v.end()));
}
public:
DynString() : c(new char[1]{}) {}
DynString(const char* s) : DynString(s, s + std::strlen(s)) {}
DynString(const DynString& s) : DynString(s.c) {}
DynString(DynString&& s) : DynString() { swap(s); }
template<class It, decltype((void)DynString(std::iterator_traits<It>::iterator_category(), first, last))...>
DynString(It first, It last) : DynString(std::iterator_traits<It>::iterator_category(), first, last) {}
~DynString() { delete[] c; }
friend ostream& operator<<(ostream& lhs, const DynString& a) { return lhs << a.c; }
char& operator[](std::size_t i) noexcept { return c[i]; }
const char& operator[](std::size_t i) const noexcept { return c[i]; }
auto data() noexcept { return c; }
auto data() const noexcept { return c; }
auto begin() noexcept { return c; }
auto begin() const noexcept { return c; }
auto end() noexcept { return c + size(); }
auto end() const noexcept { return c + size(); }
auto rbegin() noexcept { return std::make_reverse_iterator(end()); }
auto rbegin() const noexcept { return std::make_reverse_iterator(end()); }
auto rend() noexcept { return std::make_reverse_iterator(begin()); }
auto rend() const noexcept { return std::make_reverse_iterator(begin()); }
auto size() const noexcept { return std::strlen(c); }
friend DynString operator+(const DynString& a, const DynString& b)
{ return {std::piecewise_construct, a.c, b.c}; }
friend DynString operator+(const char* a, const DynString& b)
{ return {std::piecewise_construct, a, b.c}; }
friend DynString operator+(const DynString& a, const char* b)
{ return {std::piecewise_construct, a.c, b}; }
DynString& operator+=(const DynString& s) { return *this += s.c; }
DynString& operator+=(const char* s) { return *this = *this + s; }
DynString& operator=(DynString s) { swap(s); return *this; }
void swap(DynString& o) noexcept { using std::swap; swap(c, o.c); }
};
#include <cstring>
#include <algorithm>
#include <utility>
#include <iterator>
#include <vector>
class DynString {
char *c;
DynString(std::picewise_construct_t, const char* a, const char* b) : c() {
auto na = std::strlen(a), nb = std::strlen(b);
c = new char[a + b + 1 > a ? a + b + 1 : SIZE_MAX];
std::copy_n(a, na, c);
std::copy_n(b, nb + 1, c + na);
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::forward_iterator_tag, It first, It last) : c() {
auto n = std::distance(first, last);
c = new char[0 <= n && n < SIZE_MAX ? std::size_t(n) + 1 : SIZE_MAX];
std::copy_n(first, std::size_t(n), c);
c[n] = 0;
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::input_iterator_tag, It first, It last) : c() {
std::vector<char> v(first, last);
swap({v.begin(), v.end()});
}
public:
DynString() : c(new char[1]{}) {}
DynString(const char* s) : DynString(s, s + std::strlen(s)) {}
DynString(const DynString& s) : DynString(s.c) {}
DynString(DynString&& s) : DynString() { swap(s); }
template<class It, decltype((void)DynString(std::iterator_traits<It>::iterator_category(), first, last))...>
DynString(It first, It last) : DynString(std::iterator_traits<It>::iterator_category(), first, last) {}
~DynString() { delete[] c; }
friend ostream& operator<<(ostream& lhs, const DynString& a) { return lhs << a.c; }
char& operator[](std::size_t i) noexcept { return c[i]; }
const char& operator[](std::size_t i) const noexcept { return c[i]; }
auto data() noexcept { return c; }
auto data() const noexcept { return c; }
auto begin() noexcept { return c; }
auto begin() const noexcept { return c; }
auto end() noexcept { return c + size(); }
auto end() const noexcept { return c + size(); }
auto rbegin() noexcept { return std::make_reverse_iterator(end()); }
auto rbegin() const noexcept { return std::make_reverse_iterator(end()); }
auto rend() noexcept { return std::make_reverse_iterator(begin()); }
auto rend() const noexcept { return std::make_reverse_iterator(begin()); }
auto size() const noexcept { return std::strlen(c); }
friend DynString operator+(const DynString& a, const DynString& b)
{ return {std::piecewise_construct, a.c, b.c}; }
friend DynString operator+(const char* a, const DynString& b)
{ return {std::piecewise_construct, a, b.c}; }
friend DynString operator+(const DynString& a, const char* b)
{ return {std::piecewise_construct, a.c, b}; }
DynString& operator+=(const DynString& s) { return *this += s.c; }
DynString& operator+=(const char* s) { return *this = *this + s; }
DynString& operator=(DynString s) { swap(s); return *this; }
void swap(DynString& o) noexcept { using std::swap; swap(c, o.c); }
};
Modified code, deciding that nullptr
is UB, and throwing in some extras:
#include <cstring>
#include <algorithm>
#include <utility>
#include <iterator>
#include <vector>
class DynString {
char *c;
DynString(std::picewise_construct_t, const char* a, const char* b) {
auto na = std::strlen(a), nb = std::strlen(b);
c = new char[a + b + 1 > a ? a + b + 1 : SIZE_MAX];
std::copy_n(a, na, c);
std::copy_n(b, nb + 1, c + na);
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::forward_iterator_tag, It first, It last) {
auto n = std::distance(first, last);
c = new char[0 <= n && n < SIZE_MAX ? std::size_t(n) + 1 : SIZE_MAX];
try {
std::copy_n(first, std::size_t(n), c);
c[n] = 0;
} catch(...) {
delete [] c;
throw;
}
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::input_iterator_tag, It first, It last) : c() {
std::vector<char> v(first, last);
swap(DynString(v.begin(), v.end()));
}
public:
DynString() : c(new char[1]{}) {}
DynString(const char* s) : DynString(s, s + std::strlen(s)) {}
DynString(const DynString& s) : DynString(s.c) {}
DynString(DynString&& s) : DynString() { swap(s); }
template<class It, decltype((void)DynString(std::iterator_traits<It>::iterator_category(), first, last))...>
DynString(It first, It last) : DynString(std::iterator_traits<It>::iterator_category(), first, last) {}
~DynString() { delete[] c; }
friend ostream& operator<<(ostream& lhs, const DynString& a) { return lhs << a.c; }
char& operator[](std::size_t i) noexcept { return c[i]; }
const char& operator[](std::size_t i) const noexcept { return c[i]; }
auto data() noexcept { return c; }
auto data() const noexcept { return c; }
auto begin() noexcept { return c; }
auto begin() const noexcept { return c; }
auto end() noexcept { return c + size(); }
auto end() const noexcept { return c + size(); }
auto rbegin() noexcept { return std::make_reverse_iterator(end()); }
auto rbegin() const noexcept { return std::make_reverse_iterator(end()); }
auto rend() noexcept { return std::make_reverse_iterator(begin()); }
auto rend() const noexcept { return std::make_reverse_iterator(begin()); }
auto size() const noexcept { return std::strlen(c); }
friend DynString operator+(const DynString& a, const DynString& b)
{ return {std::piecewise_construct, a.c, b.c}; }
friend DynString operator+(const char* a, const DynString& b)
{ return {std::piecewise_construct, a, b.c}; }
friend DynString operator+(const DynString& a, const char* b)
{ return {std::piecewise_construct, a.c, b}; }
DynString& operator+=(const DynString& s) { return *this += s.c; }
DynString& operator+=(const char* s) { return *this = *this + s; }
DynString& operator=(DynString s) { swap(s); return *this; }
void swap(DynString& o) noexcept { using std::swap; swap(c, o.c); }
};
Modified code, deciding that nullptr
is UB, and throwing in some extras:
#include <cstring>
#include <algorithm>
#include <utility>
#include <iterator>
#include <vector>
class DynString {
char *c;
DynString(std::picewise_construct_t, const char* a, const char* b) {
auto na = std::strlen(a), nb = std::strlen(b);
c = new char[a + b + 1 > a ? a + b + 1 : SIZE_MAX];
std::copy_n(a, na, c);
std::copy_n(b, nb + 1, c + na);
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::forward_iterator_tag, It first, It last) {
auto n = std::distance(first, last);
c = new char[0 <= n && n < SIZE_MAX ? std::size_t(n) + 1 : SIZE_MAX];
try {
std::copy_n(first, std::size_t(n), c);
c[n] = 0;
} catch(...) {
delete [] c;
throw;
}
}
template<class It, decltype((void)(*c = *std::declval<It>()))...>
DynString(std::input_iterator_tag, It first, It last) : c() {
std::vector<char> v(first, last);
swap(DynString(v.begin(), v.end()));
}
public:
DynString() : c(new char[1]{}) {}
DynString(const char* s) : DynString(s, s + std::strlen(s)) {}
DynString(const DynString& s) : DynString(s.c) {}
DynString(DynString&& s) : DynString() { swap(s); }
template<class It, decltype((void)DynString(std::iterator_traits<It>::iterator_category(), first, last))...>
DynString(It first, It last) : DynString(std::iterator_traits<It>::iterator_category(), first, last) {}
~DynString() { delete[] c; }
friend ostream& operator<<(ostream& lhs, const DynString& a) { return lhs << a.c; }
char& operator[](std::size_t i) noexcept { return c[i]; }
const char& operator[](std::size_t i) const noexcept { return c[i]; }
auto data() noexcept { return c; }
auto data() const noexcept { return c; }
auto begin() noexcept { return c; }
auto begin() const noexcept { return c; }
auto end() noexcept { return c + size(); }
auto end() const noexcept { return c + size(); }
auto rbegin() noexcept { return std::make_reverse_iterator(end()); }
auto rbegin() const noexcept { return std::make_reverse_iterator(end()); }
auto rend() noexcept { return std::make_reverse_iterator(begin()); }
auto rend() const noexcept { return std::make_reverse_iterator(begin()); }
auto size() const noexcept { return std::strlen(c); }
friend DynString operator+(const DynString& a, const DynString& b)
{ return {std::piecewise_construct, a.c, b.c}; }
friend DynString operator+(const char* a, const DynString& b)
{ return {std::piecewise_construct, a, b.c}; }
friend DynString operator+(const DynString& a, const char* b)
{ return {std::piecewise_construct, a.c, b}; }
DynString& operator+=(const DynString& s) { return *this += s.c; }
DynString& operator+=(const char* s) { return *this = *this + s; }
DynString& operator=(DynString s) { swap(s); return *this; }
void swap(DynString& o) noexcept { using std::swap; swap(c, o.c); }
};