[Mono-dev] Bug 10784

Alexander Köplinger alexander.koeplinger at xamarin.com
Wed Jan 6 22:22:36 UTC 2016


Please open multiple pull requests for unrelated commits, it's easier to
keep track that way.

We do the actual code review comments on GitHub, but you can use the
mailing list for discussion if you want.

- Alex

2016-01-06 19:17 GMT+01:00 Jamie Venning <
jamie_venning at technologyonecorp.com>:

> I'll make those changes and submit the pull request, though it may take a
> day or two for me to be able to sign the CLA.
> As for testing this 'properly' -- I gave it a go and it was just as
> ridiculous as you'd think and I had a hard time making it break.
>
> If I am fixing multiple bugs/issues/etc, is it ok for me to lump them into
> pull requests of 2-3 issues (1 commit per issue) or is it preferred to have
> unrelated commits in separate pull requests? (assume that I don't mind
> commits being held up by a pull request being knocked back for
> revisions/improvements) And forgive me, but I am new to mailing lists --
> should I send an email for every pull request? Is the mailing list the
> preferred place to do things like code-review?
>
> On 7 January 2016 at 02:49, Andi McClure <andi.mcclure at xamarin.com> wrote:
>
>> Jamie:
>>
>> A good place to start would be with running the automated tests to see if
>> there are any new failures. You can quickly run the most relevant tests for
>> this change with:
>> make -C mono/mini check
>> make -C mono/tests check
>> make -C mcs/class/corlib check
>> If you run `make check` at the toplevel, this will run all tests. This
>> will take a pretty long time however (and if you submit a pull request our
>> jenkins bot will do it for you).
>>
>> We normally accept patches through pull requests to mono/mono on Github.
>> When your patch is ready (which it seems to me to basically be, if the
>> tests pass) you should sign the CLA ( https://cla.xamarin.com/ ) and
>> then open a pull request so it can be merged.
>>
>> Two comments I would leave if I saw this as a pull request are:
>>     g_assert (iid <= 1000000000);
>> This number is pretty arbitrary. Please make a constant like
>> LOADED_INTERFACES_MAX to hold it instead of just putting it inline the
>> code. Also, I think you might as well bump the value up to 2^31-2, since
>> that's the highest number there's a technical reason not to go above…
>> Also, "however haven't bitset code still uses" seems to be a typo.
>>     g_assert(icount < 65535);
>> This can and I think should be <=
>>
>> I wonder if there's a non-ridiculous way to add a new automated test for
>> a program with >65535 interfaces.
>>
>> Thanks
>>
>> On Tue, Jan 5, 2016 at 9:04 PM, Jamie Venning <
>> jamie_venning at technologyonecorp.com> wrote:
>>
>>> I've gone through and changed interface_id to a guint32, along with
>>> max_interface_id, leaving interface_count (I didn't see the need for it to
>>> be increased).
>>>
>>> I've forked the repo and have it over at
>>> https://github.com/tastywheattasteslikechicken/mono/commit/6e7f1d94eb07822fb7bf5687b67df5076f969818
>>> Are there any other considerations or things that may be missed?
>>>
>>> On Wed, Jan 6, 2016 at 12:51 AM, Jamie Venning <
>>> jamie_venning at technologyonecorp.com> wrote:
>>>
>>>> Yeah, what I'm chasing are the best ways to cause failures without this
>>>> assertion -- that way as I go through and fix things I can be more
>>>> confident that things haven't been missed.
>>>>
>>>> On Tue, Jan 5, 2016 at 11:45 PM, Alexander Köplinger <
>>>> alexander.koeplinger at xamarin.com> wrote:
>>>>
>>>>> I don't think it's as easy as removing the assert.
>>>>>
>>>>> For one, the interface ID returned by mono_get_unique_iid() is stored
>>>>> as a guint16 (
>>>>> https://github.com/mono/mono/blob/36c7332104eb5250a93079ae77c2e0dbf12c6c9a/mono/metadata/class-internals.h#L344)
>>>>> which means you only have 0-65,535 values there. I'm not familiar enough
>>>>> with the runtime codebase to say which other problems you'll run into if
>>>>> you try to fix this.
>>>>>
>>>>> - Alex
>>>>>
>>>>> 2016-01-05 8:38 GMT+01:00 Jamie Venning <
>>>>> jamie_venning at technologyonecorp.com>:
>>>>>
>>>>>> Good Morning.
>>>>>>
>>>>>> Bug 10784 (
>>>>>> https://bugzilla.xamarin.com/show_bug.cgi?format=multiple&id=10784)
>>>>>> is a major blocker for me, and a big obstacle for larger applications
>>>>>> moving from .NET to Mono. Because I need this fixed, I am prepared to fix
>>>>>> it myself.
>>>>>>
>>>>>> I'm wondering if anyone knowledgeable in this area could answer these
>>>>>> few, short questions just to confirm a few things.
>>>>>>
>>>>>> Removing the offending assertion from class.c, and running generated
>>>>>> test cases that previously triggered it, I did not notice any incorrect
>>>>>> behaviour in terms of reflection, the 'is' operator failing, or the wrong
>>>>>> implementation of functions being used. Is there anywhere that I should be
>>>>>> looking in particular or any particular type of tests I can use to expose
>>>>>> an underlying fault that removing this assertion causes?
>>>>>>
>>>>>> Is it possible that this assertion is vestigial or unnecessary?
>>>>>>
>>>>>> --
>>>>>>
>>>>>> James Venning
>>>>>> Research & Development - Ci2 - Product
>>>>>> Phone: +61 7 3167 7300 | Fax: +61 7 3167 7301 Address: Level 11,
>>>>>> TechnologyOne HQ, 540 Wickham Street, Fortitude Valley QLD 4006 (PO Box 96
>>>>>> Fortitude Valley QLD 4006) Email: jamie_venning at technologyonecorp.com
>>>>>> Web: TechnologyOneCorp.com <http://www.technologyonecorp.com/>
>>>>>>
>>>>>> <http://www.technologyonecorp.com/>
>>>>>>
>>>>>> Financials | Human Resource & Payroll | Supply Chain | Corporate
>>>>>> Performance Management | Property & Rating | Student Management | Asset
>>>>>> Management | Enterprise Content Management | Customer Relationship
>>>>>> Management | Consulting Services
>>>>>>
>>>>>> *TechnologyOne (ASX:TNE) is Australia's largest publicly listed
>>>>>> software company, with offices across six countries including each state
>>>>>> and territory of Australia, as well as New Zealand, the South Pacific, Asia
>>>>>> and the United Kingdom. For 25 years, we have been providing powerful and
>>>>>> deeply integrated enterprise software solutions that are used every day by
>>>>>> more than 1000 leading corporations, government departments and statutory
>>>>>> authorities.*
>>>>>>
>>>>>> TechnologyOne accepts no liability for any damage caused by this
>>>>>> email or its attachments. The information in this email is only for the
>>>>>> intended recipient and may contain confidential and/or privileged material.
>>>>>> If you received this in error, please kindly notify the sender and delete
>>>>>> this email and any attachments from your system. Opinions, conclusions and
>>>>>> other information in this message that do not relate to the official
>>>>>> business of the company shall be understood as neither given nor endorsed
>>>>>> by it. If you believe that you have been spammed please email
>>>>>> Stop_Spam at TechnologyOneCorp.com <stop_spam at technologyonecorp.com>.
>>>>>>
>>>>>> _______________________________________________
>>>>>> Mono-devel-list mailing list
>>>>>> Mono-devel-list at lists.ximian.com
>>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Jamie Venning
>>>> Research & Development - Ci2 - Product
>>>> Phone: +61 7 3167 7300 | Fax: +61 7 3167 7301 Address: Level 11,
>>>> TechnologyOne HQ, 540 Wickham Street, Fortitude Valley QLD 4006 (PO Box 96
>>>> Fortitude Valley QLD 4006) Email: jamie_venning at technologyonecorp.com
>>>> Web: TechnologyOneCorp.com <http://www.technologyonecorp.com/>
>>>>
>>>> <http://www.technologyonecorp.com/>
>>>>
>>>> Financials | Human Resource & Payroll | Supply Chain | Corporate
>>>> Performance Management | Property & Rating | Student Management | Asset
>>>> Management | Enterprise Content Management | Customer Relationship
>>>> Management | Consulting Services
>>>>
>>>> *TechnologyOne (ASX:TNE) is Australia's largest publicly listed
>>>> software company, with offices across six countries including each state
>>>> and territory of Australia, as well as New Zealand, the South Pacific, Asia
>>>> and the United Kingdom. For 25 years, we have been providing powerful and
>>>> deeply integrated enterprise software solutions that are used every day by
>>>> more than 1000 leading corporations, government departments and statutory
>>>> authorities.*
>>>>
>>>> TechnologyOne accepts no liability for any damage caused by this email
>>>> or its attachments. The information in this email is only for the intended
>>>> recipient and may contain confidential and/or privileged material. If you
>>>> received this in error, please kindly notify the sender and delete this
>>>> email and any attachments from your system. Opinions, conclusions and other
>>>> information in this message that do not relate to the official business of
>>>> the company shall be understood as neither given nor endorsed by it. If you
>>>> believe that you have been spammed please email
>>>> Stop_Spam at TechnologyOneCorp.com <stop_spam at technologyonecorp.com>.
>>>>
>>>
>>>
>>>
>>> --
>>>
>>> Jamie Venning
>>> Intern Developer | Research & Development - Ci2 - Product
>>> Phone: +61 7 3167 7300 | Fax: +61 7 3167 7301 Address: Level 11,
>>> TechnologyOne HQ, 540 Wickham Street, Fortitude Valley QLD 4006 (PO Box 96
>>> Fortitude Valley QLD 4006) Email: jamie_venning at technologyonecorp.com
>>> Web: TechnologyOneCorp.com <http://www.technologyonecorp.com/>
>>>
>>> <http://www.technologyonecorp.com/>
>>>
>>> Financials | Human Resource & Payroll | Supply Chain | Corporate
>>> Performance Management | Property & Rating | Student Management | Asset
>>> Management | Enterprise Content Management | Customer Relationship
>>> Management | Consulting Services
>>>
>>> *TechnologyOne (ASX:TNE) is Australia's largest publicly listed software
>>> company, with offices across six countries including each state and
>>> territory of Australia, as well as New Zealand, the South Pacific, Asia and
>>> the United Kingdom. For 25 years, we have been providing powerful and
>>> deeply integrated enterprise software solutions that are used every day by
>>> more than 1000 leading corporations, government departments and statutory
>>> authorities.*
>>>
>>> TechnologyOne accepts no liability for any damage caused by this email
>>> or its attachments. The information in this email is only for the intended
>>> recipient and may contain confidential and/or privileged material. If you
>>> received this in error, please kindly notify the sender and delete this
>>> email and any attachments from your system. Opinions, conclusions and other
>>> information in this message that do not relate to the official business of
>>> the company shall be understood as neither given nor endorsed by it. If you
>>> believe that you have been spammed please email
>>> Stop_Spam at TechnologyOneCorp.com <stop_spam at technologyonecorp.com>.
>>>
>>> _______________________________________________
>>> Mono-devel-list mailing list
>>> Mono-devel-list at lists.ximian.com
>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>
>>>
>>
>
>
> --
>
> Jamie Venning
> Intern Developer | Research & Development - Ci2 - Product
> Phone: +61 7 3167 7300 | Fax: +61 7 3167 7301 Address: Level 11,
> TechnologyOne HQ, 540 Wickham Street, Fortitude Valley QLD 4006 (PO Box 96
> Fortitude Valley QLD 4006) Email: jamie_venning at technologyonecorp.com
> Web: TechnologyOneCorp.com <http://www.technologyonecorp.com/>
>
> <http://www.technologyonecorp.com/>
>
> Financials | Human Resource & Payroll | Supply Chain | Corporate
> Performance Management | Property & Rating | Student Management | Asset
> Management | Enterprise Content Management | Customer Relationship
> Management | Consulting Services
>
> *TechnologyOne (ASX:TNE) is Australia's largest publicly listed software
> company, with offices across six countries including each state and
> territory of Australia, as well as New Zealand, the South Pacific, Asia and
> the United Kingdom. For 25 years, we have been providing powerful and
> deeply integrated enterprise software solutions that are used every day by
> more than 1000 leading corporations, government departments and statutory
> authorities.*
>
> TechnologyOne accepts no liability for any damage caused by this email or
> its attachments. The information in this email is only for the intended
> recipient and may contain confidential and/or privileged material. If you
> received this in error, please kindly notify the sender and delete this
> email and any attachments from your system. Opinions, conclusions and other
> information in this message that do not relate to the official business of
> the company shall be understood as neither given nor endorsed by it. If you
> believe that you have been spammed please email
> Stop_Spam at TechnologyOneCorp.com <stop_spam at technologyonecorp.com>.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20160106/35ecf48e/attachment-0001.html>


More information about the Mono-devel-list mailing list