Skip to main content
Code Review

Return to Answer

deleted 38 characters in body
Source Link
Deduplicator
  • 19.9k
  • 1
  • 32
  • 65
  1. 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.

  2. 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?

  3. 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.

  4. 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.

  5. 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 of std::unique_ptr.

  6. Your nullptr-handling is inconsistent. Is nullptr UB, an empty string, or preserved as a separate state?
    Both of the latter two make sense.

  7. As an aside, prefer nullptr to NULL or 0. Not only does it have a distinctive type, it also cannot be confused with an integer.

  8. Use std::size_t for indices and sizes. That's what it's for.

  9. You forgot to mark your functions noexcept where allowed.

  10. Your default-constructor should be marked constexpr, or allocate a 0-length string.

  11. In DynString(const char*) you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficient memcpy instead of doing a less performant strcpy?
    At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark it explicit.

  12. DynString(const DynString&) should delegate to DynString(const char*) instead of writing everything out all over again.

  13. You are missing a move-ctor.

  14. The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.

  15. You are missing a constant overload for operator []. And anyway, the index should be a std::size_t, not an int.

  16. Your operator+(DynString&) (and operator+(const char*) too) is a first-grade mess:

  1. 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.

  2. 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?

  3. 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.

  4. 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.

  5. 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 of std::unique_ptr.

  6. Your nullptr-handling is inconsistent. Is nullptr UB, an empty string, or preserved as a separate state?
    Both of the latter two make sense.

  7. As an aside, prefer nullptr to NULL or 0. Not only does it have a distinctive type, it also cannot be confused with an integer.

  8. Use std::size_t for indices and sizes. That's what it's for.

  9. You forgot to mark your functions noexcept where allowed.

  10. Your default-constructor should be marked constexpr, or allocate a 0-length string.

  11. In DynString(const char*) you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficient memcpy instead of doing a less performant strcpy?
    At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark it explicit.

  12. DynString(const DynString&) should delegate to DynString(const char*) instead of writing everything out all over again.

  13. You are missing a move-ctor.

  14. The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.

  15. You are missing a constant overload for operator []. And anyway, the index should be a std::size_t, not an int.

  16. Your operator+(DynString&) (and operator+(const char*) too) is a first-grade mess:

  1. 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.

  2. 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?

  3. 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.

  4. 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.

  5. 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 of std::unique_ptr.

  6. Your nullptr-handling is inconsistent. Is nullptr UB, an empty string, or preserved as a separate state?

  7. As an aside, prefer nullptr to NULL or 0. Not only does it have a distinctive type, it also cannot be confused with an integer.

  8. Use std::size_t for indices and sizes. That's what it's for.

  9. You forgot to mark your functions noexcept where allowed.

  10. Your default-constructor should be marked constexpr, or allocate a 0-length string.

  11. In DynString(const char*) you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficient memcpy instead of doing a less performant strcpy?
    At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark it explicit.

  12. DynString(const DynString&) should delegate to DynString(const char*) instead of writing everything out all over again.

  13. You are missing a move-ctor.

  14. The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.

  15. You are missing a constant overload for operator []. And anyway, the index should be a std::size_t, not an int.

  16. Your operator+(DynString&) (and operator+(const char*) too) is a first-grade mess:

deleted 112 characters in body
Source Link
Deduplicator
  • 19.9k
  • 1
  • 32
  • 65
#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); }
};
added 3421 characters in body
Source Link
Deduplicator
  • 19.9k
  • 1
  • 32
  • 65

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); }
};
added 69 characters in body
Source Link
Deduplicator
  • 19.9k
  • 1
  • 32
  • 65
Loading
Source Link
Deduplicator
  • 19.9k
  • 1
  • 32
  • 65
Loading
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /