Merge pull request #1511 from Febbe/fix-undefined-behaviour

Fix undefined behaviour

src/model/Point.h

@@ -32,6 +32,8 @@ public:
 	 */
 	Point(const Point& p);
 
+	Point& operator=(Point const&) = default;
+
 	/**
 	 * @brief Point from two values.
 	 * @param x X value of the point.

src/model/Stroke.cpp

@@ -6,6 +6,7 @@
 #include <i18n.h>
 
 #include <cmath>
+#include <numeric>
 
 Stroke::Stroke()
  : AudioElement(ELEMENT_STROKE)
@@ -17,11 +18,6 @@ Stroke::~Stroke()
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	g_free(this->points);
-	this->points = NULL;
-	this->pointCount = 0;
-	this->pointAllocCount = 0;
-
 	XOJ_RELEASE_TYPE(Stroke);
 }
 
@@ -45,11 +41,7 @@ Stroke* Stroke::cloneStroke() const
 
 	Stroke* s = new Stroke();
 	s->applyStyleFrom(this);
-
+	s->points = this->points;
-	s->allocPointSize(this->pointCount);
-	memcpy(s->points, this->points, this->pointCount * sizeof(Point));
-	s->pointCount = this->pointCount;
-
 	return s;
 }
 
@@ -74,7 +66,7 @@ void Stroke::serialize(ObjectOutputStream& out)
 
 	out.writeInt(fill);
 
-	out.writeData(this->points, this->pointCount, sizeof(Point));
+	out.writeData(this->points.data(), this->points.size(), sizeof(Point));
 
 	this->lineStyle.serialize(out);
 
@@ -95,11 +87,11 @@ void Stroke::readSerialized(ObjectInputStream& in)
 
 	this->fill = in.readInt();
 
-	g_free(this->points);
+	Point* p;
-	this->points = NULL;
+	int count{};
-	this->pointCount = 0;
+	in.readData((void**) &p, &count);
-	in.readData((void**) &this->points, &this->pointCount);
+	this->points = std::vector<Point>{p, p + count};
-
+	delete[] p;
 	this->lineStyle.readSerialized(in);
 
 	in.endObject();
@@ -151,10 +143,10 @@ bool Stroke::isInSelection(ShapeContainer* container)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	for (int i = 0; i < this->pointCount; i++)
+	for (auto&& p: this->points)
 	{
-		double px = this->points[i].x;
+		double px = p.x;
-		double py = this->points[i].y;
+		double py = p.y;
 
 		if (!container->contains(px, py))
 		{
@@ -169,9 +161,9 @@ void Stroke::setFirstPoint(double x, double y)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointCount > 0)
+	if (!this->points.empty())
 	{
-		Point& p = this->points[0];
+		Point& p = this->points.front();
 		p.x = x;
 		p.y = y;
 		this->sizeCalculated = false;
@@ -181,114 +173,77 @@ void Stroke::setFirstPoint(double x, double y)
 void Stroke::setLastPoint(double x, double y)
 {
 	XOJ_CHECK_TYPE(Stroke);
-
+	setLastPoint({x, y});
-	if (this->pointCount > 0)
-	{
-		Point& p = this->points[this->pointCount - 1];
-		p.x = x;
-		p.y = y;
-		this->sizeCalculated = false;
-	}
 }
 
-void Stroke::setLastPoint(Point p)
+void Stroke::setLastPoint(const Point& p)
 {
-	setLastPoint(p.x, p.y);
+	if (!this->points.empty())
-}
-
-void Stroke::addPoint(Point p)
-{
-	XOJ_CHECK_TYPE(Stroke);
-
-	if (this->pointCount >= this->pointAllocCount - 1)
 	{
-		this->allocPointSize(this->pointAllocCount + 100);
+		this->points.back() = p;
 	}
-	this->points[this->pointCount++] = p;
-	this->sizeCalculated = false;
 }
 
-void Stroke::allocPointSize(int size)
+void Stroke::addPoint(const Point& p)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	this->pointAllocCount = size;
+	this->points.emplace_back(p);
-	this->points = (Point*) g_realloc(this->points, this->pointAllocCount * sizeof(Point));
+	this->sizeCalculated = false;
 }
 
 int Stroke::getPointCount() const
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	return this->pointCount;
+	return this->points.size();
 }
 
 ArrayIterator<Point> Stroke::pointIterator() const
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	return ArrayIterator<Point> (points, pointCount);
+	return ArrayIterator<Point>(points.data(), points.size());
 }
 
 void Stroke::deletePointsFrom(int index)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointCount <= index)
+	points.resize(std::min(size_t(index), points.size()));
-	{
-		return;
-	}
-	this->pointCount = index;
 }
 
 void Stroke::deletePoint(int index)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointCount <= index)
+	this->points.erase(std::next(begin(this->points), index));
-	{
-		return;
-	}
-
-	for (int i = 0; i < this->pointCount; i++)
-	{
-		if (i >= index)
-		{
-			this->points[i] = this->points[i + 1];
-		}
-	}
-	this->pointCount--;
 }
 
 Point Stroke::getPoint(int index) const
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (index < 0 || index >= pointCount)
+	if (index < 0 || index >= this->points.size())
 	{
 		g_warning("Stroke::getPoint(%i) out of bounds!", index);
 		return Point(0, 0, Point::NO_PRESSURE);
 	}
-	return points[index];
+	return points.at(index);
 }
 
 const Point* Stroke::getPoints() const
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	return this->points;
+	return this->points.data();
 }
 
 void Stroke::freeUnusedPointItems()
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointAllocCount == this->pointCount)
+	this->points = {begin(this->points), end(this->points)};
-	{
-		return;
-	}
-	this->pointAllocCount = this->pointCount + 1;
-	this->points = (Point*) g_realloc(this->points, this->pointAllocCount * sizeof(Point));
 }
 
 void Stroke::setToolType(StrokeTool type)
@@ -323,10 +278,10 @@ void Stroke::move(double dx, double dy)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	for (int i = 0; i < pointCount; i++)
+	for (auto&& point: points)
 	{
-		points[i].x += dx;
+		point.x += dx;
-		points[i].y += dy;
+		point.y += dy;
 	}
 
 	this->sizeCalculated = false;
@@ -335,11 +290,9 @@ void Stroke::move(double dx, double dy)
 void Stroke::rotate(double x0, double y0, double xo, double yo, double th)
 {
 	XOJ_CHECK_TYPE(Stroke);
-	
-	for (int i = 0; i < this->pointCount; i++)
-	{
-		Point& p = this->points[i];
 
+	for (auto&& p: points)
+	{
 		p.x -= x0;	//move to origin
 		p.y -= y0;
 		double offset = 0.7; // __DBL_EPSILON__;
@@ -354,8 +307,8 @@ void Stroke::rotate(double x0, double y0, double xo, double yo, double th)
 		p.x += x0;	//restore the position
 		p.y += y0;
 
-		p.x += xo-offset;	//center it
+		p.x += xo - offset;  //center it
-		p.y += yo-offset;		
+		p.y += yo - offset;
 	}
 	//Width and Height will likely be changed after this operation
 	calcSize();
@@ -367,10 +320,8 @@ void Stroke::scale(double x0, double y0, double fx, double fy)
 
 	double fz = sqrt(fx * fy);
 
-	for (int i = 0; i < this->pointCount; i++)
+	for (auto&& p: points)
 	{
-		Point& p = this->points[i];
-		
 		p.x -= x0;
 		p.x *= fx;
 		p.x += x0;
@@ -393,7 +344,7 @@ bool Stroke::hasPressure() const
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointCount > 0)
+	if (!this->points.empty())
 	{
 		return this->points[0].z != Point::NO_PRESSURE;
 	}
@@ -403,12 +354,9 @@ bool Stroke::hasPressure() const
 double Stroke::getAvgPressure() const
 {
 	XOJ_CHECK_TYPE(Stroke);
-	double summatory = 0;
+	return std::accumulate(begin(this->points), end(this->points), 0.0,
-	for (int i = 0; i < this->pointCount; i++)
+	                       [](double l, Point const& p) { return l + p.z; }) /
-	{
+	       this->points.size();
-		summatory += this->points[i].z;
-	}
-	return summatory / this->pointCount;
 }
 
 void Stroke::scalePressure(double factor)
@@ -419,9 +367,9 @@ void Stroke::scalePressure(double factor)
 	{
 		return;
 	}
-	for (int i = 0; i < this->pointCount; i++)
+	for (auto&& p: this->points)
 	{
-		this->points[i].z *= factor;
+		p.z *= factor;
 	}
 }
 
@@ -429,9 +377,9 @@ void Stroke::clearPressure()
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	for (int i = 0; i < this->pointCount; i++)
+	for (auto&& p: points)
 	{
-		this->points[i].z = Point::NO_PRESSURE;
+		p.z = Point::NO_PRESSURE;
 	}
 }
 
@@ -439,9 +387,9 @@ void Stroke::setLastPressure(double pressure)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointCount > 0)
+	if (!this->points.empty())
 	{
-		this->points[this->pointCount - 1].z = pressure;
+		this->points.back().z = pressure;
 	}
 }
 
@@ -450,13 +398,14 @@ void Stroke::setPressure(const vector<double>& pressure)
 	XOJ_CHECK_TYPE(Stroke);
 
 	// The last pressure is not used - as there is no line drawn from this point
-	if (this->pointCount - 1 > (int)pressure.size())
+	if (this->points.size() - 1 != pressure.size())
 	{
-		g_warning("invalid pressure point count: %i, expected %i", (int)pressure.size(), (int)this->pointCount - 1);
+		g_warning("invalid pressure point count: %s, expected %s", std::to_string(pressure.size()).data(),
-		return;
+		          std::to_string(this->points.size() - 1).data());
 	}
 
-	for (int i = 0; i < this->pointCount && i < (int)pressure.size(); i++)
+	auto max_size = std::min(pressure.size(), this->points.size() - 1);
+	for (size_t i = 0U; i != max_size; ++i)
 	{
 		this->points[i].z = pressure[i];
 	}
@@ -479,7 +428,7 @@ bool Stroke::intersects(double x, double y, double halfEraserSize, double* gap)
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointCount < 1)
+	if (this->points.empty())
 	{
 		return false;
 	}
@@ -491,10 +440,10 @@ bool Stroke::intersects(double x, double y, double halfEraserSize, double* gap)
 
 	double lastX = points[0].x;
 	double lastY = points[0].y;
-	for (int i = 1; i < pointCount; i++)
+	for (auto&& point: points)
 	{
-		double px = points[i].x;
+		double px = point.x;
-		double py = points[i].y;
+		double py = point.y;
 
 		if (px >= x1 && py >= y1 && px <= x2 && py <= y2)
 		{
@@ -556,7 +505,7 @@ void Stroke::calcSize()
 {
 	XOJ_CHECK_TYPE(Stroke);
 
-	if (this->pointCount == 0)
+	if (this->points.empty())
 	{
 		Element::x = 0;
 		Element::y = 0;
@@ -574,15 +523,15 @@ void Stroke::calcSize()
 	bool hasPressure = points[0].z != Point::NO_PRESSURE;
 	double halfThick = this->width / 2.0;  //  accommodate for pen width
 
-	for (int i = 0; i < this->pointCount; i++)
+	for (auto&& p: points)
 	{
-		if (hasPressure) halfThick = points[i].z / 2.0;
+		if (hasPressure) halfThick = p.z / 2.0;
 
-		minX = std::min(minX, points[i].x - halfThick);
+		minX = std::min(minX, p.x - halfThick);
-		minY = std::min(minY, points[i].y - halfThick);
+		minY = std::min(minY, p.y - halfThick);
 
-		maxX = std::max(maxX, points[i].x + halfThick);
+		maxX = std::max(maxX, p.x + halfThick);
-		maxY = std::max(maxY, points[i].y + halfThick);
+		maxY = std::max(maxY, p.y + halfThick);
 	}
 
 	Element::x = minX - 2;
@@ -611,9 +560,8 @@ void Stroke::debugPrint()
 
 	g_message("%s", FC(FORMAT_STR("Stroke {1} / hasPressure() = {2}") % (uint64_t) this % this->hasPressure()));
 
-	for (int i = 0; i < this->pointCount; i++)
+	for (auto&& p: points)
 	{
-		Point p = this->points[i];
 		g_message("%lf / %lf", p.x, p.y);
 	}
 

src/model/Stroke.h

@@ -29,6 +29,11 @@ class Stroke : public AudioElement
 {
 public:
 	Stroke();
+	Stroke(Stroke const&) = default;
+	Stroke(Stroke&&) = default;
+
+	Stroke& operator=(Stroke const&) = default;
+	Stroke& operator=(Stroke&&) = default;
 	virtual ~Stroke();
 
 public:
@@ -61,10 +66,10 @@ public:
 	 */
 	void setFill(int fill);
 
-	void addPoint(Point p);
+	void addPoint(const Point& p);
 	void setLastPoint(double x, double y);
 	void setFirstPoint(double x, double y);
-	void setLastPoint(Point p);
+	void setLastPoint(const Point& p);
 	int getPointCount() const;
 	void freeUnusedPointItems();
 	ArrayIterator<Point> pointIterator() const;
@@ -109,7 +114,6 @@ public:
 
 protected:
 	virtual void calcSize();
-	void allocPointSize(int size);
 
 private:
 	XOJ_TYPE_ATTRIB;
@@ -120,9 +124,7 @@ private:
 	StrokeTool toolType = STROKE_TOOL_PEN;
 
 	// The array with the points
-	Point* points = NULL;
+	std::vector<Point> points{};
-	int pointCount = 0;
-	int pointAllocCount = 0;
 
 	/**
 	 * Dashed line