[LLVMdev] qsort callbacks portability issues

[Chris Lattner]

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