Improved Guard APIs codegen

[Sergio0694]

The JIT assumes that forward branches are always taken, and with the custom ThrowHelper methods used in the Guard APIs having the [MethodImpl] attributes, it couldn't see that they were always throwing, so the resulting codegen was not optimal. I've inverted all the conditions so that the forward branch is the likely one (the one that doesn't fault). This allowes the JIT to properly optimize the codegen in methods using the Guard APIs, eg. by only using jumps for the faulting paths and placing them further down in the code, with the likely branch not having to jump instead. These are the same changes done in the Microsoft.Toolkit.Diagnostics.Guard class, see CommunityToolkit/WindowsCommunityToolkit#3323.

NOTE: @JimBobSquarePants not sure how to generate that .tt.bak file, could you tell me what steps I should do here?

[JimBobSquarePants]

not sure how to generate that .tt.bak file, could you tell me what steps I should do here?

Easy peasy... I simply renamed the .tt file after I'd generated the classes from it! 😄

[Sergio0694]

Oh...
Not sure why, I saw that .bak extension and just assumed it must've had to do with some custom build script or something 🤣

Well that worked, PR should be good to go now, thanks! 🎉

[JimBobSquarePants]

I'm gonna see if I can get CI up and running for this PR to improve test visibility.

[JimBobSquarePants]

@MarcoRossignoli I hate to do this but I can't figure out why the code coverage reports here are empty?

Would you be able to take a quick look when you are able, no rush! Thanks!

Scratch that, figured it out! Because we are including the shared project directly into the test project I had to configure the .runsettings file in the following manner.

<?xml version="1.0" encoding="utf-8" ?>
<RunSettings>
    <DataCollectionRunSettings>
        <DataCollectors>
            <DataCollector friendlyName="XPlat code coverage">
                <Configuration>
                    <Format>lcov</Format>
                    <ExcludeByFile>**/tests/**/*.cs, **/**/*.Program.cs</ExcludeByFile>
                    <IncludeTestAssembly>true</IncludeTestAssembly>
                </Configuration>
            </DataCollector>
        </DataCollectors>
    </DataCollectionRunSettings>
</RunSettings>

https://github.com/SixLabors/SharedInfrastructure/pull/12/files#diff-7134c7037c9f1d02b0e4ea0be9828609R1-R14

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@b0d4cd9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #12   +/-   ##
=========================================
  Coverage          ?   19.77%           
=========================================
  Files             ?        4           
  Lines             ?      359           
  Branches          ?       88           
=========================================
  Hits              ?       71           
  Misses            ?      288           
  Partials          ?        0           

Flag	Coverage Δ
#unittests	19.77% <0.00%> (?)

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

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0d4cd9...d7e37f1. Read the comment docs.

[JimBobSquarePants]

@Sergio0694 Our code coverage is appalling. I'd like to do some work on that before merging.

[Sergio0694]

@JimBobSquarePants Are we missing tests for ThrowHelper or for Guard too?
I could port/adapt some of those I have in the toolkit (see here)?

Also, side question - do we need another PR to update the submodule reference for ImageSharp after this is merged?
Not super familiar with submodules myself so not sure if there's some automation there or something 😅

[JimBobSquarePants]

@Sergio0694 Guard and DebugGuard should cover everything we need as they call ThrowHelper

You can see the coverage here.
https://codecov.io/gh/SixLabors/SharedInfrastructure/pull/12/tree

Once we've finished, yep it's a new PR for each consuming repo. Hopefully this is the last change we'll see for a while.

[MarcoRossignoli]

Hi guys!

Scratch that, figured it out! Because we are including the shared project directly into the test project I had to configure the .runsettings file in the following manner.

Yep test project is not included in coverage by default

Let me know if need further assistance

[JimBobSquarePants]

@MarcoRossignoli Thanks mate! Once I'd remembered how my project worked it was dead easy. Love how simple the configuration options are!