llvm dev - Dec 2010 - [LLVMdev] qsort callbacks portability issues

Hi,

there are a couple of possible issues around array_pod_sort
from STLExtras.h .

Firstly, qsort() needs a comparator that returns zero on equal
inputs.  ConstantIntSortPredicate in SimplifyCFG.cpp (where
the sort is being used to bring duplicate values together)
fails this requirement. All the others look ok.

Secondly, on Windows qsort() needs its callback to use __cdecl
calling convention, which won't be the case by default if
LLVM is being built with e.g. __fastcall.  VC++ will fault
this as an invalid conversion.  To make this work, the
comparator functions should be marked up with __cdecl.
It looks like this would be a few places in STLExtras.h, and
two custom comparators elsewhere.  It wouldn't be a breaking
change.  I was wondering what the preferred way of doing this
would be?  Conditionalizing each case on _MSC_VER is ugly,
but the ugliness is very local, while factoring it out into a
portability header as some kind of SORT_CALLBACK macro would
be cleaner but more pervasive.

Al


-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium.  Thank you.

On Dec 14, 2010, at 4:24 AM, Alasdair Grant wrote:
> Hi,
> 
> there are a couple of possible issues around array_pod_sort
> from STLExtras.h .
> 
> Firstly, qsort() needs a comparator that returns zero on equal
> inputs.  ConstantIntSortPredicate in SimplifyCFG.cpp (where
> the sort is being used to bring duplicate values together)
> fails this requirement. All the others look ok.
Fixed in r121838, thanks for pointing this out!
> Secondly, on Windows qsort() needs its callback to use __cdecl
> calling convention, which won't be the case by default if
> LLVM is being built with e.g. __fastcall.
How likely is building everything as fastcall-by-default to work beyond this? 
I'm highly skeptical that such a thing would work.
> VC++ will fault
> this as an invalid conversion.  To make this work, the
> comparator functions should be marked up with __cdecl.
> It looks like this would be a few places in STLExtras.h, and
> two custom comparators elsewhere.  It wouldn't be a breaking
> change.  I was wondering what the preferred way of doing this
> would be?  Conditionalizing each case on _MSC_VER is ugly,
> but the ugliness is very local, while factoring it out into a
> portability header as some kind of SORT_CALLBACK macro would
> be cleaner but more pervasive.
Why not make the array_pod_sort template wrap the comparator in a function call
with the right abi?  Again, is this really an issue?

-Chris