shiboken2: Allow specifying the sequence of overloads

Add an attribute to specify a number by which the functions
will be sorted. This deactivates the default sorting
which tries to avoid implicit conversions.

Fixes: PYSIDE-1366
Change-Id: I9a891e21f86152b2fdfda9a48d685f19aa936508
Reviewed-by: Christian Tismer <tismer@stackless.com>
Reviewed-by: Cristian Maureira-Fredes <cristian.maureira-fredes@qt.io>

13 files changed, 142 insertions, 1 deletions

diff --git a/sources/shiboken2/ApiExtractor/abstractmetalang.cpp b/sources/shiboken2/ApiExtractor/abstractmetalang.cpp
index d20caacf1..e93108be1 100644
--- a/sources/shiboken2/ApiExtractor/abstractmetalang.cpp
+++ b/sources/shiboken2/ApiExtractor/abstractmetalang.cpp
@@ -1283,6 +1283,21 @@ AbstractMetaFunction::find(const AbstractMetaFunctionList &haystack,
     return findByName(haystack, needle);
 }
 
+int AbstractMetaFunction::overloadNumber() const
+{
+    if (m_cachedOverloadNumber == TypeSystem::OverloadNumberUnset) {
+        m_cachedOverloadNumber = TypeSystem::OverloadNumberDefault;
+        const FunctionModificationList &mods = modifications(implementingClass());
+        for (const FunctionModification &mod : mods) {
+            if (mod.overloadNumber() != TypeSystem::OverloadNumberUnset) {
+                m_cachedOverloadNumber = mod.overloadNumber();
+                break;
+            }
+        }
+    }
+    return m_cachedOverloadNumber;
+}
+
 #ifndef QT_NO_DEBUG_STREAM
 static inline void formatMetaFunctionBrief(QDebug &d, const AbstractMetaFunction *af)
 {
diff --git a/sources/shiboken2/ApiExtractor/abstractmetalang.h b/sources/shiboken2/ApiExtractor/abstractmetalang.h
index 9ceae14df..2fe114de4 100644
--- a/sources/shiboken2/ApiExtractor/abstractmetalang.h
+++ b/sources/shiboken2/ApiExtractor/abstractmetalang.h
@@ -1084,6 +1084,8 @@ public:
     void setExceptionHandlingModification(TypeSystem::ExceptionHandling em)
     { m_exceptionHandlingModification = em;  }
 
+     int overloadNumber() const;
+
 #ifndef QT_NO_DEBUG_STREAM
     void formatDebugVerbose(QDebug &d) const;
 #endif
@@ -1115,6 +1117,7 @@ private:
     uint m_explicit                 : 1;
     uint m_pointerOperator          : 1;
     uint m_isCallOperator           : 1;
+    mutable int m_cachedOverloadNumber = TypeSystem::OverloadNumberUnset;
     ExceptionSpecification m_exceptionSpecification = ExceptionSpecification::Unknown;
     TypeSystem::AllowThread m_allowThreadModification = TypeSystem::AllowThread::Unspecified;
     TypeSystem::ExceptionHandling m_exceptionHandlingModification = TypeSystem::ExceptionHandling::Unspecified;
diff --git a/sources/shiboken2/ApiExtractor/typesystem.h b/sources/shiboken2/ApiExtractor/typesystem.h
index b31cee9cd..f351699d8 100644
--- a/sources/shiboken2/ApiExtractor/typesystem.h
+++ b/sources/shiboken2/ApiExtractor/typesystem.h
@@ -356,6 +356,9 @@ struct FunctionModification: public Modification
     TypeSystem::ExceptionHandling exceptionHandling() const { return m_exceptionHandling; }
     void setExceptionHandling(TypeSystem::ExceptionHandling e) { m_exceptionHandling = e; }
 
+    int overloadNumber() const { return m_overloadNumber; }
+    void setOverloadNumber(int overloadNumber) { m_overloadNumber = overloadNumber; }
+
     QString toString() const;
 
 #ifndef QT_NO_DEBUG_STREAM
@@ -371,6 +374,7 @@ private:
     QString m_signature;
     QString m_originalSignature;
     QRegularExpression m_signaturePattern;
+    int m_overloadNumber = TypeSystem::OverloadNumberUnset;
     bool m_thread = false;
     AllowThread m_allowThread = AllowThread::Unspecified;
     TypeSystem::ExceptionHandling m_exceptionHandling = TypeSystem::ExceptionHandling::Unspecified;
diff --git a/sources/shiboken2/ApiExtractor/typesystem_enums.h b/sources/shiboken2/ApiExtractor/typesystem_enums.h
index f6b4b6fa6..0d7f279c4 100644
--- a/sources/shiboken2/ApiExtractor/typesystem_enums.h
+++ b/sources/shiboken2/ApiExtractor/typesystem_enums.h
@@ -88,6 +88,8 @@ enum Visibility { // For namespaces
     Auto
 };
 
+enum : int { OverloadNumberUnset = -1, OverloadNumberDefault = 99999 };
+
 } // namespace TypeSystem
 
 #endif // TYPESYSTEM_ENUMS_H
diff --git a/sources/shiboken2/ApiExtractor/typesystemparser.cpp b/sources/shiboken2/ApiExtractor/typesystemparser.cpp
index 6979a7a4e..44616da0a 100644
--- a/sources/shiboken2/ApiExtractor/typesystemparser.cpp
+++ b/sources/shiboken2/ApiExtractor/typesystemparser.cpp
@@ -76,6 +76,7 @@ static inline QString invalidateAfterUseAttribute() { return QStringLiteral("inv
 static inline QString locationAttribute() { return QStringLiteral("location"); }
 static inline QString modifiedTypeAttribute() { return QStringLiteral("modified-type"); }
 static inline QString modifierAttribute() { return QStringLiteral("modifier"); }
+static inline QString overloadNumberAttribute() { return QStringLiteral("overload-number"); }
 static inline QString ownershipAttribute() { return QStringLiteral("owner"); }
 static inline QString packageAttribute() { return QStringLiteral("package"); }
 static inline QString positionAttribute() { return QStringLiteral("position"); }
@@ -2151,6 +2152,18 @@ bool TypeSystemParser::parseModifyField(const QXmlStreamReader &reader,
     return true;
 }
 
+static bool parseOverloadNumber(const QXmlStreamAttribute &attribute, int *overloadNumber,
+                                QString *errorMessage)
+{
+    bool ok;
+    *overloadNumber = attribute.value().toInt(&ok);
+    if (!ok || *overloadNumber < 0) {
+        *errorMessage = msgInvalidAttributeValue(attribute);
+        return false;
+    }
+    return true;
+}
+
 bool TypeSystemParser::parseAddFunction(const QXmlStreamReader &,
                                const StackElement &topElement,
                                QXmlStreamAttributes *attributes)
@@ -2164,6 +2177,7 @@ bool TypeSystemParser::parseAddFunction(const QXmlStreamReader &,
     QString returnType = QLatin1String("void");
     bool staticFunction = false;
     QString access;
+    int overloadNumber = TypeSystem::OverloadNumberUnset;
     for (int i = attributes->size() - 1; i >= 0; --i) {
         const QStringRef name = attributes->at(i).qualifiedName();
         if (name == QLatin1String("signature")) {
@@ -2175,6 +2189,9 @@ bool TypeSystemParser::parseAddFunction(const QXmlStreamReader &,
                                             staticAttribute(), false);
         } else if (name == accessAttribute()) {
             access = attributes->takeAt(i).value().toString();
+        } else if (name == overloadNumberAttribute()) {
+            if (!parseOverloadNumber(attributes->takeAt(i), &overloadNumber, &m_error))
+                return false;
         }
     }
 
@@ -2210,6 +2227,7 @@ bool TypeSystemParser::parseAddFunction(const QXmlStreamReader &,
         m_contextStack.top()->functionMods.size();
 
     FunctionModification mod;
+    mod.setOverloadNumber(overloadNumber);
     if (!mod.setSignature(m_currentSignature, &m_error))
         return false;
     mod.setOriginalSignature(originalSignature);
@@ -2234,6 +2252,7 @@ bool TypeSystemParser::parseModifyFunction(const QXmlStreamReader &reader,
     QString association;
     bool deprecated = false;
     bool isThread = false;
+    int overloadNumber = TypeSystem::OverloadNumberUnset;
     TypeSystem::ExceptionHandling exceptionHandling = TypeSystem::ExceptionHandling::Unspecified;
     TypeSystem::AllowThread allowThread = TypeSystem::AllowThread::Unspecified;
     for (int i = attributes->size() - 1; i >= 0; --i) {
@@ -2270,6 +2289,9 @@ bool TypeSystemParser::parseModifyFunction(const QXmlStreamReader &reader,
                 qCWarning(lcShiboken, "%s",
                           qPrintable(msgInvalidAttributeValue(attribute)));
             }
+        } else if (name == overloadNumberAttribute()) {
+            if (!parseOverloadNumber(attributes->takeAt(i), &overloadNumber, &m_error))
+                return false;
         } else if (name == virtualSlotAttribute()) {
             qCWarning(lcShiboken, "%s",
                       qPrintable(msgUnimplementedAttributeWarning(reader, name)));
@@ -2293,6 +2315,7 @@ bool TypeSystemParser::parseModifyFunction(const QXmlStreamReader &reader,
         return false;
     mod.setOriginalSignature(originalSignature);
     mod.setExceptionHandling(exceptionHandling);
+    mod.setOverloadNumber(overloadNumber);
     m_currentSignature = signature;
 
     if (!access.isEmpty()) {
diff --git a/sources/shiboken2/doc/typesystem_manipulating_objects.rst b/sources/shiboken2/doc/typesystem_manipulating_objects.rst
index f76289bc4..a5ba24c09 100644
--- a/sources/shiboken2/doc/typesystem_manipulating_objects.rst
+++ b/sources/shiboken2/doc/typesystem_manipulating_objects.rst
@@ -103,6 +103,7 @@ modify-function
                               access="public | private | protected"
                               allow-thread="true | auto | false"
                               exception-handling="off | auto-off | auto-on | on"
+                              overload-number="number"
                               rename="..." />
          </object-type>
 
@@ -132,6 +133,47 @@ modify-function
              declares ``noexcept``
            * yes, true: Always generate exception handling code
 
+    The optional ``overload-number`` attribute specifies the position of the
+    overload when checking arguments. Typically, when a number of overloads
+    exists, as for in example in Qt:
+
+    .. code-block:: c++
+
+        void QPainter::drawLine(QPointF, QPointF);
+        void QPainter::drawLine(QPoint, QPoint);
+
+    they will be reordered such that the check for matching arguments for the
+    one taking a ``QPoint`` is done first. This is to avoid a potentially
+    costly implicit conversion from ``QPoint`` to ``QPointF`` when using the
+    2nd overload. There are cases though in which this is not desired;
+    most prominently when a class inherits from a container and overloads exist
+    for both types as is the case for the ``QPolygon`` class:
+
+    .. code-block:: c++
+
+        class QPolygon : public QList<QPoint> {};
+
+        void QPainter::drawPolygon(QPolygon);
+        void QPainter::drawPolygon(QList<QPoint>);
+
+    By default, the overload taking a ``QList`` will be checked first, trying
+    to avoid constructing a ``QPolygon`` from ``QList``. The type check for a
+    list of points will succeed for a parameter of type ``QPolygon``, too,
+    since it inherits ``QList``. This presents a problem since the sequence
+    type check is costly due to it checking that each container element is a
+    ``QPoint``. It is thus preferable to check for the ``QPolygon`` overload
+    first. This is achieved by specifying numbers as follows:
+
+    .. code-block:: xml
+
+        <object-type name="QPainter">
+            <modify-function signature="drawPolygon(QPolygon)" overload-number="0"/>
+            <modify-function signature="drawPolygon(QList&lt;QPoint&gt;)" overload-number="1"/>
+        </object-type>
+
+    Numbers should be given for all overloads; otherwise, the order will be in
+    declaration order.
+
     The ``remove``, ``access`` and ``rename`` attributes are *optional* attributes
     for added convenience; they serve the same purpose as the deprecated tags :ref:`remove`, :ref:`access` and :ref:`rename`.
 
diff --git a/sources/shiboken2/generator/shiboken2/overloaddata.cpp b/sources/shiboken2/generator/shiboken2/overloaddata.cpp
index e70eeaea1..36725d3fc 100644
--- a/sources/shiboken2/generator/shiboken2/overloaddata.cpp
+++ b/sources/shiboken2/generator/shiboken2/overloaddata.cpp
@@ -38,6 +38,8 @@
 #include <QtCore/QFile>
 #include <QtCore/QTemporaryFile>
 
+#include <algorithm>
+
 static const TypeEntry *getReferencedTypeEntry(const TypeEntry *typeEntry)
 {
     if (typeEntry->isPrimitive()) {
@@ -175,6 +177,24 @@ static QString msgCyclicDependency(const QString &funcName, const QString &graph
     return result;
 }
 
+static inline int overloadNumber(const OverloadData *o)
+{
+    return o->referenceFunction()->overloadNumber();
+}
+
+bool OverloadData::sortByOverloadNumberModification()
+{
+    if (std::all_of(m_nextOverloadData.cbegin(), m_nextOverloadData.cend(),
+                    [](const OverloadData *o) { return overloadNumber(o) == TypeSystem::OverloadNumberDefault; })) {
+        return false;
+    }
+    std::stable_sort(m_nextOverloadData.begin(), m_nextOverloadData.end(),
+                     [] (const OverloadData *o1, const OverloadData *o2) {
+                         return overloadNumber(o1) < overloadNumber(o2);
+                     });
+    return true;
+}
+
 /**
  * Topologically sort the overloads by implicit convertion order
  *
@@ -210,7 +230,7 @@ void OverloadData::sortNextOverloads()
     for (OverloadData *ov : qAsConst(m_nextOverloadData))
         ov->sortNextOverloads();
 
-    if (m_nextOverloadData.size() <= 1)
+    if (m_nextOverloadData.size() <= 1 || sortByOverloadNumberModification())
         return;
 
     // Populates the OverloadSortData object containing map and reverseMap, to map type names to ids,
diff --git a/sources/shiboken2/generator/shiboken2/overloaddata.h b/sources/shiboken2/generator/shiboken2/overloaddata.h
index 4fd4199e5..9ffb084ff 100644
--- a/sources/shiboken2/generator/shiboken2/overloaddata.h
+++ b/sources/shiboken2/generator/shiboken2/overloaddata.h
@@ -139,6 +139,7 @@ private:
     OverloadData *addOverloadData(const AbstractMetaFunction *func, const AbstractMetaArgument *arg);
 
     void sortNextOverloads();
+    bool sortByOverloadNumberModification();
 
     int functionNumber(const AbstractMetaFunction *func) const;
     OverloadDataList overloadDataOnPosition(OverloadData *overloadData, int argPos) const;
diff --git a/sources/shiboken2/tests/libsample/overloadsort.cpp b/sources/shiboken2/tests/libsample/overloadsort.cpp
index bad0cdf52..8534ef0f1 100644
--- a/sources/shiboken2/tests/libsample/overloadsort.cpp
+++ b/sources/shiboken2/tests/libsample/overloadsort.cpp
@@ -28,3 +28,12 @@
 
 #include "overloadsort.h"
 
+int CustomOverloadSequence::overload(short v) const
+{
+    return v + int(sizeof(v));
+}
+
+int CustomOverloadSequence::overload(int v) const
+{
+    return v + 4;
+}
diff --git a/sources/shiboken2/tests/libsample/overloadsort.h b/sources/shiboken2/tests/libsample/overloadsort.h
index 024f229a5..ad720222c 100644
--- a/sources/shiboken2/tests/libsample/overloadsort.h
+++ b/sources/shiboken2/tests/libsample/overloadsort.h
@@ -84,5 +84,12 @@ public:
 
 };
 
+class LIBSAMPLE_API CustomOverloadSequence
+{
+public:
+    int overload(short v) const;
+    int overload(int v) const;
+};
+
 #endif // OVERLOADSORT_H
 
diff --git a/sources/shiboken2/tests/samplebinding/CMakeLists.txt b/sources/shiboken2/tests/samplebinding/CMakeLists.txt
index 5cc7092b2..ad52565ad 100644
--- a/sources/shiboken2/tests/samplebinding/CMakeLists.txt
+++ b/sources/shiboken2/tests/samplebinding/CMakeLists.txt
@@ -22,6 +22,7 @@ ${CMAKE_CURRENT_BINARY_DIR}/sample/collector_wrapper.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/sample/comparisontester_wrapper.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/sample/color_wrapper.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/sample/ctorconvrule_wrapper.cpp
+${CMAKE_CURRENT_BINARY_DIR}/sample/customoverloadsequence_wrapper.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/sample/cvlistuser_wrapper.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/sample/cvvaluetype_wrapper.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/sample/sbkdate_wrapper.cpp
diff --git a/sources/shiboken2/tests/samplebinding/overload_sorting_test.py b/sources/shiboken2/tests/samplebinding/overload_sorting_test.py
index 8132e4e3d..958aa0a6d 100644
--- a/sources/shiboken2/tests/samplebinding/overload_sorting_test.py
+++ b/sources/shiboken2/tests/samplebinding/overload_sorting_test.py
@@ -94,5 +94,14 @@ class EnumOverIntSorting(unittest.TestCase):
         ic = ImplicitConv(ImplicitConv.CtorTwo)
         self.assertEqual(ic.ctorEnum(), ImplicitConv.CtorTwo)
 
+
+class TestCustomOverloadSequence(unittest.TestCase):
+    '''Ensure the int-overload (returning v + sizeof(v)) is first as specified via
+       overload-number in XML.'''
+    def testCustomOverloadSequence(self):
+        s = CustomOverloadSequence()
+        self.assertEqual(s.overload(42), 46)
+
+
 if __name__ == '__main__':
     unittest.main()
diff --git a/sources/shiboken2/tests/samplebinding/typesystem_sample.xml b/sources/shiboken2/tests/samplebinding/typesystem_sample.xml
index a1f8cd6d1..132bff4ed 100644
--- a/sources/shiboken2/tests/samplebinding/typesystem_sample.xml
+++ b/sources/shiboken2/tests/samplebinding/typesystem_sample.xml
@@ -1806,6 +1806,11 @@
     </value-type>
     <value-type name="ImplicitTarget"/>
 
+    <object-type name="CustomOverloadSequence">
+        <modify-function signature="overload(int) const" overload-number="0"/>
+        <modify-function signature="overload(short) const" overload-number="1"/>
+    </object-type>
+
     <value-type name="Point">
         <add-function signature="__str__" return-type="PyObject*">
             <inject-code class="target" position="beginning">