Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Single-allocation method, with smart pointer

##Single-allocation method, with smart pointer WeWe can use std::unique_ptr to hold our data, if we don't mind using a custom deleter:

##Single-allocation method, with smart pointer We can use std::unique_ptr to hold our data, if we don't mind using a custom deleter:

Single-allocation method, with smart pointer

We can use std::unique_ptr to hold our data, if we don't mind using a custom deleter:

Fix the leak with try-catch to show why smart pointer is better
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
#include <cstring>
#include <string>
#include <vector>
char *const *strlist(const std::vector<std::string>& input)
{
 char **result = new char*[input.size() + 1];
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 try {
 char *storage = new char[storage_size];
 char *p = storage;
 char **q = result;
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
 }
 catch (...) {
  delete[] result;
 throw;
 }
}
void del_strlist(char *const *strings)
{
 // First string is the allocated storage
 delete[] strings[0];
 delete[] strings;
}

As well as the fragile interface, this still suffers from the same memory leak as the original, so I don't advise using this version.

You should see that this is a bit simpler to write and to use than my first version.

#include <cstring>
#include <string>
#include <vector>
char *const *strlist(const std::vector<std::string>& input)
{
 char **result = new char*[input.size() + 1];
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 char *storage = new char[storage_size];
 char *p = storage;
 char **q = result;
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
}
void del_strlist(char *const *strings)
{
 // First string is the allocated storage
 delete[] strings[0];
 delete[] strings;
}

As well as the fragile interface, this still suffers from the same memory leak as the original, so I don't advise using this version.

#include <cstring>
#include <string>
#include <vector>
char *const *strlist(const std::vector<std::string>& input)
{
 char **result = new char*[input.size() + 1];
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 try {
 char *storage = new char[storage_size];
 char *p = storage;
 char **q = result;
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
 }
 catch (...) {
  delete[] result;
 throw;
 }
}
void del_strlist(char *const *strings)
{
 // First string is the allocated storage
 delete[] strings[0];
 delete[] strings;
}

You should see that this is a bit simpler to write and to use than my first version.

Describe the memory leak, and how I fixed it
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325

Firstly, there's a memory leak possible, because strlist performs two allocations using new[]. If the first succeeds, but the second throws std::bad_alloc, then we have no reference to *result with which to delete[] it.

The answer by Incomputable shows how the interface can be greatly improved.

I have a minor point that might be missed inoverlooked when reading that answer, and an efficiency improvement that can be incorporated into the original or into the proposed RAII object.

#include <cstring>
#include <string>
#include <vector>
char *const *strlist(const std::vector<std::string>& input)
{
 char **result = new char*[input.size() + 1];
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 char *storage = new char[storage_size];
 char *p = storage;
 char **q = result;
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
}
void del_strlist(char *const *strings)
{
 // First string is the allocated storage
 delete[] strings[0];
 delete[] strings;
}

As well as the fragile interface, this still suffers from the same memory leak as the original, so I don't advise using this version.


##Single-allocation method, with smart pointer We can use std::unique_ptr to hold our data, if we don't mind using a custom deleter:

#include <cstring>
#include <memory>
#include <string>
#include <vector>
auto strlist(const std::vector<std::string>& input)
{
 static auto const deleter = [](char**p) {
 // First string is the allocated storage
 delete[] p[0];
 delete[]p;
 };
 std::unique_ptr<char*[], decltype(deleter)>
 result{new char*[input.size() + 1], deleter};
 // Ensure that destructor is safe (in case next 'new[]' fails)
 result[0] = nullptr;
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 char *p = result[0] = new char[storage_size];
 char **q = result.get();
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
}

The answer by Incomputable shows how the interface can be greatly improved.

I have a minor point that might be missed in that answer, and an efficiency improvement that can be incorporated into the original or into the proposed RAII object.

#include <cstring>
#include <string>
#include <vector>
char *const *strlist(const std::vector<std::string>& input)
{
 char **result = new char*[input.size() + 1];
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 char *storage = new char[storage_size];
 char *p = storage;
 char **q = result;
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
}
void del_strlist(char *const *strings)
{
 // First string is the allocated storage
 delete[] strings[0];
 delete[] strings;
}

##Single-allocation method, with smart pointer We can use std::unique_ptr to hold our data, if we don't mind using a custom deleter:

#include <cstring>
#include <memory>
#include <string>
#include <vector>
auto strlist(const std::vector<std::string>& input)
{
 static auto const deleter = [](char**p) {
 // First string is the allocated storage
 delete[] p[0];
 delete[]p;
 };
 std::unique_ptr<char*[], decltype(deleter)>
 result{new char*[input.size() + 1], deleter};
 result[0] = nullptr;
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 char *p = result[0] = new char[storage_size];
 char **q = result.get();
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
}

Firstly, there's a memory leak possible, because strlist performs two allocations using new[]. If the first succeeds, but the second throws std::bad_alloc, then we have no reference to *result with which to delete[] it.

The answer by Incomputable shows how the interface can be greatly improved.

I have a minor point that might be overlooked when reading that answer, and an efficiency improvement that can be incorporated into the original or into the proposed RAII object.

#include <cstring>
#include <string>
#include <vector>
char *const *strlist(const std::vector<std::string>& input)
{
 char **result = new char*[input.size() + 1];
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 char *storage = new char[storage_size];
 char *p = storage;
 char **q = result;
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
}
void del_strlist(char *const *strings)
{
 // First string is the allocated storage
 delete[] strings[0];
 delete[] strings;
}

As well as the fragile interface, this still suffers from the same memory leak as the original, so I don't advise using this version.


##Single-allocation method, with smart pointer We can use std::unique_ptr to hold our data, if we don't mind using a custom deleter:

#include <cstring>
#include <memory>
#include <string>
#include <vector>
auto strlist(const std::vector<std::string>& input)
{
 static auto const deleter = [](char**p) {
 // First string is the allocated storage
 delete[] p[0];
 delete[]p;
 };
 std::unique_ptr<char*[], decltype(deleter)>
 result{new char*[input.size() + 1], deleter};
 // Ensure that destructor is safe (in case next 'new[]' fails)
 result[0] = nullptr;
 std::size_t storage_size = 0;
 for (auto const& s: input) {
 storage_size += s.size() + 1;
 }
 char *p = result[0] = new char[storage_size];
 char **q = result.get();
 for (auto const& s: input) {
 *q++ = std::strcpy(p, s.c_str());
 p += s.size() + 1;
 }
 *q = nullptr; // terminate the list
 return result;
}
Add a note that code is fully clean
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
Loading
Show smart-pointer variant of single allocation
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
Loading
Demonstrate use of a single char[] to hold all the strings
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
Loading
data() returns a null-terminated string since C++11, not C++17 as I thought.
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
Loading
Rollback to Revision 1
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
Loading
Can use range-based for
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
Loading
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325
Loading
default

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