Skip to content

bugfix in Rational.cs, when both Nominator and Denominator equals to 0 #2453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 12, 2023
Merged

bugfix in Rational.cs, when both Nominator and Denominator equals to 0 #2453

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ddecc6
bugfix in Rational.cs
tsardaryanCamenAI May 11, 2023
9b3d84f
Update LongRational.cs
tsardaryanCamenAI May 16, 2023
c09d20e
fix for Retional.FromDouble(0)
tsardaryanCamenAI May 17, 2023
9291581
Merge branch 'main' into patch-1
JimBobSquarePants Jun 8, 2023
973e6f1
Fix implementation and tests
JimBobSquarePants Jun 12, 2023
6f3d8de
Merge branch 'main' into patch-1
JimBobSquarePants Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/ImageSharp/Primitives/Rational.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ public Rational(double value, bool bestPrecision)

this.Numerator = (uint)rational.Numerator;
this.Denominator = (uint)rational.Denominator;

if(this.Numerator == 0 && this.Denominator == 0)
Copy link
Member

@JimBobSquarePants JimBobSquarePants May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's an issue here it will have to be fixed in LongRational

Copy link
Contributor Author

@tsardaryanCamenAI tsardaryanCamenAI May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to update GPS location of ExifInformation, it accepts Retional[]. That's why I suggest this change. Anyway if you are creating Rational.FromDouble(0) Denominator shouldn't be 0.

Copy link
Contributor Author

@tsardaryanCamenAI tsardaryanCamenAI May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right, the problem is deeper on line 206 of LongRational.cs it should be

return new LongRational(0, 1);

Copy link
Member

@dlemstra dlemstra May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the correct fix. Can you also add some unit tests for this to make sure this stays fixed?

Copy link
Contributor Author

@tsardaryanCamenAI tsardaryanCamenAI May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sure will do that and will push all as a different pull request. You can close this one.

Copy link
Member

@JimBobSquarePants JimBobSquarePants May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to open a new PR. That’s just confusing. Push to this one please.

Copy link
Member

@dlemstra dlemstra May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you can also use a force push to update this PR with a new set of commits.

{
this.Denominator = 1;
}
}

/// <summary>
Expand Down