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.

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