computablee / DotMP

A collection of powerful abstractions for parallel programming in .NET with an OpenMP-like API.
https://computablee.github.io/DotMP/
GNU Lesser General Public License v2.1
29 stars 8 forks source link

Fix DotMP.Parallel.Single #57

Closed blouflashdb closed 11 months ago

blouflashdb commented 11 months ago

Closes #47

computablee commented 11 months ago

Looking nice, a couple small things before I merge.

  1. Recently (today) I went through and added comments to all of the individual exception constructors (see the main branch for how this is done in Exceptions.cs). Feel free to copy+paste the comments there and reuse them.
  2. Along with the commit that addressed the exception comments, I changed all of the references to <T> in XML comments to &lt;T&gt;. Please change that on line 54 of Exceptions.cs.
  3. Please include whitespace after the if keyword on line 658 of Parallel.cs. I need to add a formatter to the CI pipeline, that's on me. I'll open an issue on that right now.
  4. Since this is not being pulled into main, can you run the tester (make test in the root of the repo) and let me know if everything passes?

Nothing you did wrong, it's just that 1 and 2 are to due with how XML documentation changed today due to now being included in the packing process (and dotnet pack was dumping out about 200 warnings I went through and tidied up). 3 is just because of a missing code formatter in the CI pipeline, that shouldn't be an issue.

blouflashdb commented 11 months ago

Sure.

blouflashdb commented 11 months ago

The Single_works is failing because the in_for_pv field is null. I am not sure if this is a problem with the test setup or if this could even be a bug at runtime. Can you take a look?

blouflashdb commented 11 months ago

I believe a null check in get_in_for_pv is required like this:

internal bool in_for
        {
            get
            {
                if (in_for_pv == null) {
                    return false;
                }

                return in_for_pv[Parallel.GetThreadNum()];
            }
            set
            {
                in_for_pv[Parallel.GetThreadNum()] = value;
            }
        }

The reason for this is because the default constructor of WorkShare is not initializing the in_for_pv field which is only the case in For or ForReduction because there the constructor for a WorkShare object is being called which then sets the static in_for_pv field.

computablee commented 11 months ago

I agree, please go ahead and make that change and let me know if everything passes.

blouflashdb commented 11 months ago

Tests are passing.

computablee commented 11 months ago

Looks good, I am accepting the PR and labeling as hacktoberfest-accepted. Thank you very much!