Skip to content

Commit

Permalink
Remove typecasting in IsCollinear (#825)
Browse files Browse the repository at this point in the history
* Remove typecasting in `IsCollinear`

* Update the C# and Delphi versions as well

* In the C++ version, multiply unsigned numbers (so that the result just wraps, and there is no undefined behavior)

* Add test case for `IsCollinear`
  • Loading branch information
reunanen authored May 1, 2024
1 parent 33ec483 commit 3963bae
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 8 deletions.
1 change: 1 addition & 0 deletions CPP/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ endif()

set(ClipperTests_SRC
Tests/TestExportHeaders.cpp
Tests/TestIsCollinear.cpp
Tests/TestLines.cpp
Tests/TestOffsets.cpp
Tests/TestOffsetOrientation.cpp
Expand Down
8 changes: 4 additions & 4 deletions CPP/Clipper2Lib/include/clipper2/clipper.core.h
Original file line number Diff line number Diff line change
Expand Up @@ -663,10 +663,10 @@ namespace Clipper2Lib
inline bool IsCollinear(const Point<T>& pt1,
const Point<T>& sharedPt, const Point<T>& pt2) // #777
{
const double a = static_cast<double>(sharedPt.x - pt1.x);
const double b = static_cast<double>(pt2.y - sharedPt.y);
const double c = static_cast<double>(sharedPt.y - pt1.y);
const double d = static_cast<double>(pt2.x - sharedPt.x);
const auto a = static_cast<uintmax_t>(sharedPt.x - pt1.x);
const auto b = static_cast<uintmax_t>(pt2.y - sharedPt.y);
const auto c = static_cast<uintmax_t>(sharedPt.y - pt1.y);
const auto d = static_cast<uintmax_t>(pt2.x - sharedPt.x);
return a * b == c * d;
}

Expand Down
13 changes: 13 additions & 0 deletions CPP/Tests/TestIsCollinear.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#include <gtest/gtest.h>
#include "clipper2/clipper.core.h"

TEST(Clipper2Tests, TestIsCollinear) {
// a large integer not representable by double
const int64_t i = 9007199254740993;

const Clipper2Lib::Point64 pt1(0, 0);
const Clipper2Lib::Point64 sharedPt(i, i * 10);
const Clipper2Lib::Point64 pt2(i * 10, i * 100);

EXPECT_TRUE(IsCollinear(pt1, sharedPt, pt2));
}
5 changes: 2 additions & 3 deletions CSharp/Clipper2Lib/Clipper.Core.cs
Original file line number Diff line number Diff line change
Expand Up @@ -604,9 +604,8 @@ internal static double CrossProduct(Point64 pt1, Point64 pt2, Point64 pt3)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static bool IsCollinear(Point64 pt1, Point64 pt2, Point64 pt3)
{
// typecast to double to avoid potential int overflow
return (double) (pt2.X - pt1.X) * (double) (pt3.Y - pt2.Y) ==
(double) (pt2.Y - pt1.Y) * (double) (pt3.X - pt2.X);
return (pt2.X - pt1.X) * (pt3.Y - pt2.Y) ==
(pt2.Y - pt1.Y) * (pt3.X - pt2.X);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
2 changes: 1 addition & 1 deletion Delphi/Clipper2Lib/Clipper.Core.pas
Original file line number Diff line number Diff line change
Expand Up @@ -1866,7 +1866,7 @@ function IsPositive(const path: TPathD): Boolean;

function IsCollinear(const pt1, pt2, pt3: TPoint64): Boolean;
var
a,b,c,d: double; // avoids potential int overflow
a,b,c,d: Int64;
begin
a := (pt2.X - pt1.X);
b := (pt2.Y - pt1.Y);
Expand Down

0 comments on commit 3963bae

Please sign in to comment.