Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Remove compiler conditions - #288

#288
Merged
JimBobSquarePants merged 2 commits into
mainSixLabors/ImageSharp.Drawing:mainfrom
js/conditionsSixLabors/ImageSharp.Drawing:js/conditionsCopy head branch name to clipboard
Aug 16, 2023
Merged

Remove compiler conditions#288
JimBobSquarePants merged 2 commits into
mainSixLabors/ImageSharp.Drawing:mainfrom
js/conditionsSixLabors/ImageSharp.Drawing:js/conditionsCopy head branch name to clipboard

Conversation

@JimBobSquarePants

Copy link
Copy Markdown
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Removes all outdated #if...#else conditions from the code.

if (this.groupList.Count == 0)
{
goto Error;
return;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I spotted this when trying to run a benchmark. I'd deviated from clippers default behavior for some reason.

// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Numerics;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@antonfirsov I spotted this comment. Are we able to replace this code with built in APIs now?

// Adapted from:
// https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs
// If targeting .NET 5, we can call span based sort, but probably not worth it only for that API.

@antonfirsov antonfirsov Aug 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we can now delete SortUtility and use span.Sort(...) overloads instead in PolygonScanner and friends.

@JimBobSquarePants JimBobSquarePants Aug 16, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Awesome thanks! I didn't even know about the additional sort APIs. Removed.

@codecov

codecov Bot commented Aug 16, 2023

Copy link
Copy Markdown

Codecov Report

Merging #288 (b3078c1) into main (b23aa22) will increase coverage by 0%.
The diff coverage is 83%.

@@         Coverage Diff          @@
##           main   #288    +/-   ##
====================================
  Coverage    80%    80%            
====================================
  Files        99     97     -2     
  Lines      5055   4875   -180     
  Branches    919    880    -39     
====================================
- Hits       4050   3925   -125     
+ Misses      807    753    -54     
+ Partials    198    197     -1     
Flag Coverage Δ
unittests 80% <83%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/ImageSharp.Drawing/Shapes/Path.cs 75% <ø> (ø)
...mageSharp.Drawing/Shapes/PolygonClipper/Clipper.cs 97% <ø> (ø)
...arp.Drawing/Shapes/PolygonClipper/ClipperOffset.cs 100% <ø> (ø)
....Drawing/Shapes/PolygonClipper/PolygonOffsetter.cs 72% <0%> (-1%) ⬇️
...c/ImageSharp.Drawing/Utilities/NumericUtilities.cs 100% <ø> (ø)
...arp.Drawing/Shapes/Rasterization/ActiveEdgeList.cs 98% <100%> (ø)
...arp.Drawing/Shapes/Rasterization/PolygonScanner.cs 96% <100%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dlemstra
dlemstra previously approved these changes Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.