It is good to show that you have tested your code; it would be even better to add tests to the Mono test suite to serve as a regression test.
Also, I could be wrong about this, but I think there is a potential problem lurking in the new thread_priority field in _MonoInternalThread. It is declared as type "int", which in C (unlike C#) is not guaranteed to be of any particular size (it is usually the same as the system word size, meaning it would be 32 bits on a 32-bit architecture and 64 bits on a 64-bit architecture; but even that is not guaranteed). This would mean that Thread and _MonoInternalThread would not be the same size on 64-bit architectures. Changing thread_priority in _MonoInternalThread from int to gint32 would guarantee that the field is always 32 bits, matching Thread. On Thu, Nov 6, 2014 at 9:38 AM, Bent Bisballe Nyeng <[email protected]> wrote: > Hi Andres > > I have added both test code and results as a comment to the pull request. > > Kind regards > Bent Bisballe Nyeng > > On 11/06/14 15:22, Andres G. Aragoneses wrote: > >> Hey Bisballe, >> >> Thanks for putting your work as a pull request! >> >> It would be nice if you could include in it some unit tests that >> demonstrate that the feature is working correctly (this will probably >> also make the PR more likely to be reviewed/merged soon). You can find >> an example of such tests here: >> >> https://bugzilla.novell.com/show_bug.cgi?id=540524 >> >> Thanks >> >> On 06/11/14 15:16, Bent Bisballe Nyeng wrote: >> >>> I have made pull request now: https://github.com/mono/mono/pull/1391 >>> >>> I have updated the patch to work with HEAD and tested it. Everything >>> seems to work as expected. >>> >>> I'm a bit new to the whole github concept, so forgive me if I have not >>> done everything by the book ;-) >>> >>> Kind regards >>> Bent Bisballe Nyeng >>> >>> On 11/06/14 12:37, Alexander Köplinger wrote: >>> >>>> There is a PR that also claims to implement SetThreadPriority >>>> (https://github.com/mono/mono/pull/1272), but it has many other >>>> unrelated changes, so not in a state to be merged. >>>> From a quick look, your patch seems to be much more focused and thus >>>> more likely to get merged. Can you open a pull request on GitHub? >>>> >>>> -- Alex >>>> >>>> >>>> > Date: Thu, 6 Nov 2014 09:12:02 +0100 >>>> > From: [email protected] >>>> > To: [email protected] >>>> > Subject: [Mono-dev] SetThreadPriority patch for mono-3.2.8 >>>> > >>>> > Hi mono devs >>>> > >>>> > I created a patch for SetThreadPriority support in mono-3.2.8 and >>>> would >>>> > very much like som feedback on it. >>>> > It can be found here: >>>> > https://gist.github.com/aasimon/c8ae6fc3cf5d9b82b6ca >>>> > Comments are welcome both here on the list as well as on the actual >>>> gist >>>> > paste. >>>> > >>>> > Kind regards >>>> > Bent Bisballe Nyeng >>>> > _______________________________________________ >>>> > Mono-devel-list mailing list >>>> > [email protected] >>>> > http://lists.ximian.com/mailman/listinfo/mono-devel-list >>>> >>> >> >> _______________________________________________ >> Mono-devel-list mailing list >> [email protected] >> http://lists.ximian.com/mailman/listinfo/mono-devel-list >> > > _______________________________________________ > Mono-devel-list mailing list > [email protected] > http://lists.ximian.com/mailman/listinfo/mono-devel-list >
_______________________________________________ Mono-devel-list mailing list [email protected] http://lists.ximian.com/mailman/listinfo/mono-devel-list
