diff --git a/code/smartPointers/CMakeLists.txt b/code/smartPointers/CMakeLists.txt new file mode 100644 index 0000000000000000000000000000000000000000..67b466fe97c14f257487bfc0d8722d16901bbd7f --- /dev/null +++ b/code/smartPointers/CMakeLists.txt @@ -0,0 +1,17 @@ + +# Set up the project. +cmake_minimum_required( VERSION 3.1 ) +project( smartPointers LANGUAGES CXX ) + +# Set up the compilation environment. +include( "${CMAKE_CURRENT_SOURCE_DIR}/../CompilerSettings.cmake" ) + +# Create the user's executable. +add_executable( smartPointers smartPointers.cpp ) + +# Create the "solution executable". +add_executable( smartPointers.sol EXCLUDE_FROM_ALL + solution/smartPointers.sol.cpp ) +if( TARGET solution ) + add_dependencies( solution trymove.sol ) +endif() diff --git a/code/smartPointers/Makefile b/code/smartPointers/Makefile new file mode 100644 index 0000000000000000000000000000000000000000..b846f14e58b0fa9629e0227257c0cffd60d7bdec --- /dev/null +++ b/code/smartPointers/Makefile @@ -0,0 +1,11 @@ +all: smartPointers +solution: smartPointers.sol + +clean: + rm -f *o *so smartPointers *~ smartPointers.sol + +% : %.cpp + $(CXX) -g -std=c++14 -Wall -Wextra -o $@ $< + +%.sol : solution/%.sol.cpp + $(CXX) -g -std=c++14 -Wall -Wextra -o $@ $< diff --git a/code/smartPointers/README.md b/code/smartPointers/README.md new file mode 100644 index 0000000000000000000000000000000000000000..409a07593d12ac2a3f8bdca8526d2e0e7209c6d1 --- /dev/null +++ b/code/smartPointers/README.md @@ -0,0 +1,25 @@ +# Writing leak-free C++. +Here we have four code snippets that will benefit from using smart pointers. + +By replacing every explicit `new` with `make_unique` or `make_shared`, +(alternatively by explicitly instantiating smart pointers,) +we will fix memory leaks, and make most cleanup code unnecessary. + +## Prerequisites + +* Which pointer is used for what? + * Raw pointer + * [`std::unique_ptr`](https://en.cppreference.com/w/cpp/memory/unique_ptr) + * [`std::shared_ptr`](https://en.cppreference.com/w/cpp/memory/shared_ptr) +* C++-14 for `std::make_unique` / `std::make_shared`. Understand what these functions do. +* Helpful: Move semantics for `problem2()`, but can do without. + +## Instructions + +* Compile and run the program. It doesn't generate any output. +* Run with valgrind to check for leaks +``` +valgrind --leak-check=full --track-origins=yes ./smartPointers +``` +* Go through `problem1()` to `problem3()` and fix the leaks using smart pointers. +* `problem4()` is the most difficult. Skip if not enough time. diff --git a/code/smartPointers/smartPointers.cpp b/code/smartPointers/smartPointers.cpp new file mode 100644 index 0000000000000000000000000000000000000000..58c58974bfa166f0fa986b1d789e466671544a40 --- /dev/null +++ b/code/smartPointers/smartPointers.cpp @@ -0,0 +1,237 @@ +#include <algorithm> +#include <cstdlib> +#include <ctime> +#include <numeric> +#include <vector> + + +/* + * Please fix all memory leaks / ownership problems using smart pointers. + * (Verify by running the program with valgrind!) + * + * Remember that: + * - The ownership of data is expressed using unique_ptr. + * - "Observer" access without ownership is expressed using raw pointers. + * - Shared access to data is expressed using shared_ptr. + */ + + +/* -------------------------------------------------------------------------------------------- + * 1: Fix the leak using a smart pointer. + * + * Note that the arguments of sumEntries() don't need to change, as it has only read access. + * -------------------------------------------------------------------------------------------- + */ + +// Declare a function to do something with the data. No need to change it. +double sumEntries(const double* data, std::size_t size) { + return std::accumulate(data, data + size, 0); +} + +// Often, data are owned by one entity, and only used by others. Fix the leak. +void problem1() { + constexpr std::size_t arraySize = 10000; + auto data = new double[arraySize]; + + // Fill array with random stuff. + for (std::size_t i = 0; i < arraySize; ++i) { + data[i] = static_cast<double>(rand()) / RAND_MAX; + } + + sumEntries(data, arraySize); +} + + + +/* -------------------------------------------------------------------------------------------- + * 2: Storing unique_ptr in collections. + * + * Often, one has to store pointers to objects in collections. Fix the memory leaks using unique_ptr. + * Notes: + * - Factory functions should return objects either directly or using smart pointers, so users are forced to + * sort out object ownership properly. Fix the return type of the factory function. + * - The vector should own the objects, so try to store them using smart pointers. + * - Since the visitor function doesn't accept smart pointers, find a solution to pass the objects. + * Note that this works without shared_ptr! + * -------------------------------------------------------------------------------------------- + */ + +// This is a large object. We maybe shouldn't copy it, so using a pointer is advisable to pass it around. +struct LargeObject { + double fData[100000]; +}; + +// A factory function to create large objects. +LargeObject* createLargeObject() { + return new LargeObject(); +} + +// A function to do something with the objects. +void visitLargeObject(LargeObject* object) { + object->fData[0] = 1.; +} + +void problem2() { + std::vector<LargeObject*> largeObjects; + + for (unsigned int i=0; i < 10; ++i) { + auto newObj = createLargeObject(); + largeObjects.push_back(newObj); + } + + for (auto obj : largeObjects) { + visitLargeObject(obj); + } +} + + + +/* -------------------------------------------------------------------------------------------- + * 3: Shared ownership. + * + * Most of the time, ownership can be solved by having one owner (with unique_ptr) and one or + * more observers. Sometimes, we need to truly share data, though. + * + * Here is an example of a completely messed up ownership model. It leaks about 1/10 of the times + * it is invoked. + * - Verify this by running it in a loop using a command like: + * CONT=true; while $CONT; do valgrind --leak-check=full --track-origins=yes ./smartPointers 2>&1 | grep -B 5 -A 5 problem3 && CONT=false; done + * - Fix the ownership model using shared_ptr! + * - Convert the vectors to holding shared_ptr. + * - Fix the arguments of the functions. + * - Speed optimisation: + * Make sure that you don't create & destroy a shared_ptr in the for loop in problem3() and when calling processElement(). + * -------------------------------------------------------------------------------------------- + */ + +// This removes the element in the middle of the vector. +void removeMiddle(std::vector<LargeObject*>& collection) { + auto middlePosition = collection.begin() + collection.size()/2; + + // Must not delete element when erasing from collection, because it's also in the copy ... + collection.erase(middlePosition, middlePosition+1); +} + +// This removes a random element. +// Note that this leaks if the element happens to be the same +// that's removed above ... +void removeRandom(std::vector<LargeObject*>& collection) { + auto pos = collection.begin() + time(nullptr) % collection.size(); + + collection.erase(pos, pos+1); +} + +// Do something with an element. +// Just a dummy function, for you to figure out how to pass an object +// managed by a shared_ptr to a function. +void processElement(const LargeObject* /*element*/) { } + + +// We have pointers to objects in two different collections. We work a bit with +// the collections, and then we try to terminate leak free. Without a shared ownership +// model, this becomes a mess. +void problem3() { + // Let's generate a vector with 10 pointers to LargeObject + std::vector<LargeObject*> objVector(10); + for (auto& ptr : objVector) { + ptr = new LargeObject(); + } + + // Let's copy it + std::vector<LargeObject*> objVectorCopy(objVector); + + + // Now we work with the objects: + removeMiddle(objVector); + removeRandom(objVectorCopy); + // ... + // ... + for (auto elm : objVector) { + processElement(elm); + } + + + // Now try to figure out what has to be deleted. It's a mess ... + // Fix using shared_ptr, so the following code becomes unnecessary: + for (auto objPtr : objVector) { + delete objPtr; + } + + for (auto objPtr : objVectorCopy) { + // If the element is in the original collection, it was already deleted. + if (std::find(objVector.begin(), objVector.end(), objPtr) == objVector.end()) { + delete objPtr; + } + } +} + + + +/* -------------------------------------------------------------------------------------------- + * 4: Smart pointers to solve ownership problems in class members. + * *** This one is difficult. *** + * + * Class members that are pointers can become a problem. + * Firstly, if only raw pointers are used, the intended ownerships are unclear. + * Secondly, it's easy to overlook that a member has to be deleted. Here, we have an example where + * the leak may not be obvious. + * + * Tasks: + * - Why do we have a memory leak? + * Hint: Use valgrind to track down what exactly leaks here, and where it was allocated. + * Hint 2: The author made a mistake when inheriting from Base. + * - Clarify the ownership using unique_ptr. + * - Fix the mistake that was made when using inheritance. + * -------------------------------------------------------------------------------------------- + */ + +// This class doesn't have a leak. +class Base { +public: + Base(std::size_t size) : + fData(new double[size]) { } + + ~Base() { + delete[] fData; + } + +private: + double* fData; // Do we own this or not? You have to check the destructor. +}; + + +// At first sight, this also doesn't leak. +class Derived : public Base { +public: + Derived(std::size_t size) : + Base(size), + _moreData(new LargeObject()) { } + + ~Derived() { + delete _moreData; + } + +private: + LargeObject* _moreData; +}; + + +// Let's trigger a leak. +void problem4() { + Base baseObject(10000); + Derived derivedObject(10000); + Base* whyDoesThisLeak = new Derived(10000); + + delete whyDoesThisLeak; +} + + + + + +int main() { + problem1(); + problem2(); + problem3(); + problem4(); +} diff --git a/code/smartPointers/solution/smartPointers.sol.cpp b/code/smartPointers/solution/smartPointers.sol.cpp new file mode 100644 index 0000000000000000000000000000000000000000..a8cf2dea973b35447759623886a503d8fe8dcbd9 --- /dev/null +++ b/code/smartPointers/solution/smartPointers.sol.cpp @@ -0,0 +1,221 @@ +#include <algorithm> +#include <cstdlib> +#include <ctime> +#include <numeric> +#include <vector> +#include <memory> + + +/* + * Please fix all memory leaks / ownership problems using smart pointers. + * (Verify by running the program with valgrind!) + * + * Remember that: + * - The ownership of data is expressed using unique_ptr. + * - "Observer" access without ownership is expressed using raw pointers. + * - Shared access to data is expressed using shared_ptr. + */ + + +/* -------------------------------------------------------------------------------------------- + * 1: Fix the leak using a smart pointer. + * + * Note that the arguments of sumEntries() don't need to change, as it has only read access. + * -------------------------------------------------------------------------------------------- + */ + +// Declare a function to do something with the data. No need to change it. +double sumEntries(const double* data, std::size_t size) { + return std::accumulate(data, data + size, 0); +} + +// Often, data are owned by one entity, and only used by others. Fix the leak. +void problem1() { + constexpr std::size_t arraySize = 10000; + auto data = std::make_unique<double[]>(arraySize); + + // Fill array with random stuff. + for (std::size_t i = 0; i < arraySize; ++i) { + data[i] = static_cast<double>(rand()) / RAND_MAX; + } + + sumEntries(data.get(), arraySize); +} + + + +/* -------------------------------------------------------------------------------------------- + * 2: Storing unique_ptr in collections. + * + * Often, one has to store pointers to objects in collections. Fix the memory leaks using unique_ptr. + * Notes: + * - Factory functions should return objects either directly or using smart pointers, so users are forced to + * sort out object ownership properly. Fix the return type of the factory function. + * - The vector should own the objects, so try to store them using smart pointers. + * - Since the visitor function doesn't accept smart pointers, find a solution to pass the objects. + * Note that this works without shared_ptr! + * -------------------------------------------------------------------------------------------- + */ + +// This is a large object. We maybe shouldn't copy it, so using a pointer is advisable to pass it around. +struct LargeObject { + double fData[100000]; +}; + +// A factory function to create large objects. +std::unique_ptr<LargeObject> createLargeObject() { + return std::make_unique<LargeObject>(); + // or + // return std::unique_ptr<LargeObject>(new LargeObject()); +} + +// A function to do something with the objects. +void visitLargeObject(LargeObject* object) { + object->fData[0] = 1.; +} + +void problem2() { + std::vector<std::unique_ptr<LargeObject>> largeObjects; + + for (unsigned int i=0; i < 10; ++i) { + auto newObj = createLargeObject(); + largeObjects.push_back(std::move(newObj)); // Can only have one copy, so need to "give up" newObj by moving it. + } + + for (const auto& obj : largeObjects) { + visitLargeObject(obj.get()); + } +} + + + +/* -------------------------------------------------------------------------------------------- + * 3: Shared ownership. + * + * Most of the time, ownership can be solved by having one owner (with unique_ptr) and one or + * more observers. Sometimes, we need to truly share data, though. + * + * Here is an example of a completely messed up ownership model. It leaks about 1/10 of the times + * it is invoked. + * - Verify this by running it in a loop using a command like: + * CONT=true; while $CONT; do valgrind --leak-check=full --track-origins=yes ./smartPointers 2>&1 | grep -B 5 -A 5 problem3 && CONT=false; done + * - Fix the ownership model using shared_ptr! + * - Convert the vectors to holding shared_ptr. + * - Fix the arguments of the functions. + * - Speed optimisation: + * Make sure that you don't create & destroy a shared_ptr in the for loop in problem3() and when calling processElement(). + * -------------------------------------------------------------------------------------------- + */ + +// This removes the element in the middle of the vector. +void removeMiddle(std::vector<std::shared_ptr<LargeObject>>& collection) { + auto middlePosition = collection.begin() + collection.size()/2; + + // Must not delete element when erasing from collection, because it's also in the copy ... + collection.erase(middlePosition, middlePosition+1); +} + +// This removes a random element. +// Note that this leaks if the element happens to be the same +// that's removed above ... +void removeRandom(std::vector<std::shared_ptr<LargeObject>>& collection) { + auto pos = collection.begin() + time(nullptr) % collection.size(); + + collection.erase(pos, pos+1); +} + +// Do something with an element. +// Just a dummy function, for you to figure out how to pass an object +// managed by a shared_ptr to a function. +void processElement(const LargeObject* /*element*/) { } + + +// We have pointers to objects in two different collections. We work a bit with +// the collections, and then we try to terminate leak free. Without a shared ownership +// model, this becomes a mess. +void problem3() { + // Let's generate a vector with 10 pointers to LargeObject + std::vector<std::shared_ptr<LargeObject>> objVector(10); + for (auto& ptr : objVector) { + ptr.reset(new LargeObject()); + } + + // Let's copy it + std::vector<std::shared_ptr<LargeObject>> objVectorCopy(objVector); + + + // Now we work with the objects: + removeMiddle(objVector); + removeRandom(objVectorCopy); + // ... + // ... + for (const auto& elm : objVector) { + processElement(elm.get()); + } +} + + + +/* -------------------------------------------------------------------------------------------- + * 4: Smart pointers to solve ownership problems in class members. + * *** This one is difficult. *** + * + * Class members that are pointers can become a problem. + * Firstly, if only raw pointers are used, the intended ownerships are unclear. + * Secondly, it's easy to overlook that a member has to be deleted. Here, we have an example where + * the leak may not be obvious. + * + * Tasks: + * - Why do we have a memory leak? + * Hint: Use valgrind to track down what exactly leaks here, and where it was allocated. + * Hint 2: The author made a mistake when inheriting from Base. + * - Clarify the ownership using unique_ptr. + * - Fix the mistake that was made when using inheritance. + * -------------------------------------------------------------------------------------------- + */ + +// This class doesn't have a leak. +class Base { +public: + Base(std::size_t size) : + fData(new double[size]) { } + + // We *need* to declare this virtual. + // Otherwise, only the destructor of Base is called + // when we derive from this class. + virtual ~Base() = default; + +private: + std::unique_ptr<double[]> fData; // Do we own this or not? You have to check the destructor. +}; + + +// At first sight, this also doesn't leak. +class Derived : public Base { +public: + Derived(std::size_t size) : + Base(size), + _moreData(new LargeObject()) { } + +private: + std::unique_ptr<LargeObject> _moreData; +}; + + +// Let's trigger a leak. +void problem4() { + Base baseObject(10000); + Derived derivedObject(10000); + std::unique_ptr<Base> whyDoesThisLeak( new Derived(10000) ); +} + + + + + +int main() { + problem1(); + problem2(); + problem3(); + problem4(); +}