Re: [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(a)reactos.org [mailto:ros-dev- 
> >  bounces(a)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(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
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev 
>  
>  Best regards,
>  Alex Ionescu
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev