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

[Ged]

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(a)reactos.org [mailto:[e-mail]] 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(a)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(a)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(a)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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev