[ros-dev] [ros-diffs] [dchapyshev] 42012: - Samplify SwitchToThread and QueueUserWorkItem - Remove unneeded InternalWorkItemTrampoline function and QUEUE_USER_WORKITEM_CONTEXT structure - Other small changes

[Thomas Bluemel]

On an architecture where function return values e.g. modify a register 
that would otherwise be preserved?  Or that have a different return 
method depending on whether data needs to be returned?  It's like 
typecasting a function to one with a different calling method, except 
that it may work fine for *most* but not all architectures.  It's still 
bad code...

Thomas
Alex Ionescu wrote:
> I have no idea why typecasting would make it "crash" on another  
> architecture...
> 
> On 21-Jul-09, at 4:02 AM, Ged wrote:
> 
>> Dmitry do you have a reply for this?
>> I worry that you're simply copying Windows internals without  
>> thinking about
>> it.
>>
>> I agree with Thomas on this, if we can do something better then we  
>> should.
>> There's no reason to do everything in exactly the same manner just  
>> to for
>> the sake of copying. In fact, I would actually advise we do things  
>> slightly
>> differently whenever possible.
>>
>> Ged.
>>
>>
>> -----Original Message-----
>> From: ros-dev-bounces at reactos.org [mailto:ros-dev- 
>> bounces at reactos.org] On
>> Behalf Of Thomas Bluemel
>> Sent: 17 July 2009 22:53
>> To: ReactOS Development List
>> Subject: Re: [ros-dev] [ros-diffs] [dchapyshev] 42012: - Samplify
>> SwitchToThread and QueueUserWorkItem - Remove unneeded
>> InternalWorkItemTrampoline function and QUEUE_USER_WORKITEM_CONTEXT
>> structure - Other small changes
>>
>> Just because Wine and Windows do it that way doesn't make it any more
>> correct or portable.
>>
>> RtlQueueWorkItem expects a function of this type:
>> typedef VOID (NTAPI *WORKERCALLBACKFUNC)(IN PVOID Context);
>>
>> The supplied function is of this type:
>> typedef ULONG (NTAPI *PTHREAD_START_ROUTINE)(PVOID Parameter);
>>
>> This doesn't crash on x86 and "works" because the return value is  
>> simply
>> ignored, but it's definitely not portable and MAY crash on another
>> architecture where it does make a difference.  I don't know if it  
>> would
>> matter on ARM/PPC/... but the old code at least implemented it
>> correctly/portable.  It's not really an issue but IMO a bad hack.
>> Typecasting a function pointer to a function with a different  
>> signature
>> is hardly ever good practice.
>>
>> - Thomas
>>
>> Dmitry Chapyshev wrote:
>>> On Fri, 17 Jul 2009 23:03:18 +0400, Thomas Bluemel <thomas at reactsoft.com 
>>> wrote:
>>>
>>>> This explains why we are using a trampoline function and not just
>>>> typecast there...  Is there a reason you changed this?
>>>>
>>>> - Thomas
>>>>
>>>> dchapyshev at svn.reactos.org wrote:
>>>>> -    /* NOTE: Don't use Function directly since the callback  
>>>>> signature
>>>>> -             differs. This might cause problems on certain
>>>>> platforms... */
>>>>> -    Status = RtlQueueWorkItem(InternalWorkItemTrampoline,
>>>>> -                              WorkItemContext,
>>>> _______________________________________________
>>>> Ros-dev mailing list
>>>> Ros-dev at reactos.org
>>>> http://www.reactos.org/mailman/listinfo/ros-dev
>>>
>>>
>>> Wine and Windows do so. Why we should do in another way? Other  
>>> reasons are
>>> necessary?
>>>
>> _______________________________________________
>> Ros-dev mailing list
>> Ros-dev at reactos.org
>> http://www.reactos.org/mailman/listinfo/ros-dev
>>
>> _______________________________________________
>> Ros-dev mailing list
>> Ros-dev at reactos.org
>> http://www.reactos.org/mailman/listinfo/ros-dev
> 
> Best regards,
> Alex Ionescu
> 
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev