Optimize BMShape::construct() and avoid UBs

Replace insane static QMap<{const char *, int}, int> with switch-case loop

Change-Id: Iddd1640eee2b0fb78923b3011bd92cc488f7cd6d
Reviewed-by: Rebecca Worledge <rebecca.worledge@theqtcompany.com>

4 files changed, 35 insertions, 59 deletions

diff --git a/src/bodymovin/bmconstants_p.h b/src/bodymovin/bmconstants_p.h
index 046e633..a88b49d 100644
--- a/src/bodymovin/bmconstants_p.h
+++ b/src/bodymovin/bmconstants_p.h
@@ -55,20 +55,6 @@
 
 #define BM_EFFECT_FILL          0x20000
 
-#define BM_SHAPE_ELLIPSE_STR    "el"
-#define BM_SHAPE_FILL_STR       "fl"
-#define BM_SHAPE_GFILL_STR      "gf"
-#define BM_SHAPE_GSTROKE_STR    "gs"
-#define BM_SHAPE_GROUP_STR      "gr"
-#define BM_SHAPE_RECT_STR       "rc"
-#define BM_SHAPE_ROUND_STR      "rd"
-#define BM_SHAPE_SHAPE_STR      "sh"
-#define BM_SHAPE_STAR_STR       "sr"
-#define BM_SHAPE_STROKE_STR     "st"
-#define BM_SHAPE_TRIM_STR       "tm"
-#define BM_SHAPE_TRANSFORM_STR  "tr"
-#define BM_SHAPE_REPEATER_STR   "rp"
-
 QT_BEGIN_NAMESPACE
 
 Q_DECLARE_LOGGING_CATEGORY(lcLottieQtBodymovinParser);
diff --git a/src/bodymovin/bmshape.cpp b/src/bodymovin/bmshape.cpp
index f65c554..982e071 100644
--- a/src/bodymovin/bmshape.cpp
+++ b/src/bodymovin/bmshape.cpp
@@ -48,10 +48,6 @@
 
 QT_BEGIN_NAMESPACE
 
-const QMap<QLatin1String, int> BMShape::m_shapeMap =
-        BMShape::setShapeMap();
-
-
 BMShape::BMShape(const BMShape &other)
     : BMBase(other)
 {
@@ -65,116 +61,111 @@ BMBase *BMShape::clone() const
     return new BMShape(*this);
 }
 
-QMap<QLatin1String, int> BMShape::setShapeMap()
-{
-    QMap<QLatin1String, int> shapeMap;
-    shapeMap.insert(QLatin1String(BM_SHAPE_ELLIPSE_STR), BM_SHAPE_ELLIPSE_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_FILL_STR), BM_SHAPE_FILL_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_GFILL_STR), BM_SHAPE_GFILL_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_GSTROKE_STR), BM_SHAPE_GSTROKE_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_GROUP_STR), BM_SHAPE_GROUP_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_RECT_STR), BM_SHAPE_RECT_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_ROUND_STR), BM_SHAPE_ROUND_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_SHAPE_STR), BM_SHAPE_SHAPE_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_STAR_STR), BM_SHAPE_STAR_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_STROKE_STR), BM_SHAPE_STROKE_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_TRIM_STR), BM_SHAPE_TRIM_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_TRANSFORM_STR), BM_SHAPE_TRANS_IX);
-    shapeMap.insert(QLatin1String(BM_SHAPE_REPEATER_STR), BM_SHAPE_REPEATER_IX);
-    return shapeMap;
-}
-
 BMShape *BMShape::construct(QJsonObject definition, BMBase *parent)
 {
     qCDebug(lcLottieQtBodymovinParser) << "BMShape::construct()";
 
     BMShape *shape = nullptr;
-    QByteArray type = definition.value(QLatin1String("ty")).toVariant().toByteArray();
+    const QByteArray type = definition.value(QLatin1String("ty")).toString().toLatin1();
+
+    if (Q_UNLIKELY(type.size() != 2)) {
+        qCWarning(lcLottieQtBodymovinParser) << "Unsupported shape type:"
+                                             << type;
+        return shape;
+    }
 
-    int typeToBuild = m_shapeMap.value(QLatin1String(type.data()), -1);
+#define BM_SHAPE_TAG(c1, c2) int((quint32(c1)<<8) | quint32(c2))
+
+    int typeToBuild = BM_SHAPE_TAG(type[0], type[1]);
 
     switch (typeToBuild) {
-    case BM_SHAPE_GROUP_IX:
+    case BM_SHAPE_TAG('g', 'r'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse group";
         shape = new BMGroup(definition, parent);
         shape->setType(BM_SHAPE_GROUP_IX);
         break;
     }
-    case BM_SHAPE_RECT_IX:
+    case BM_SHAPE_TAG('r', 'c'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse m_rect";
         shape = new BMRect(definition, parent);
         shape->setType(BM_SHAPE_RECT_IX);
         break;
     }
-    case BM_SHAPE_FILL_IX:
+    case BM_SHAPE_TAG('f', 'l'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse fill";
         shape = new BMFill(definition, parent);
         shape->setType(BM_SHAPE_FILL_IX);
         break;
     }
-    case BM_SHAPE_GFILL_IX:
+    case BM_SHAPE_TAG('g', 'f'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse group fill";
         shape = new BMGFill(definition, parent);
         shape->setType(BM_SHAPE_GFILL_IX);
         break;
     }
-    case BM_SHAPE_STROKE_IX:
+    case BM_SHAPE_TAG('s', 't'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse stroke";
         shape = new BMStroke(definition, parent);
         shape->setType(BM_SHAPE_STROKE_IX);
         break;
     }
-    case BM_SHAPE_TRANS_IX:
+    case BM_SHAPE_TAG('t', 'r'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse shape transform";
         shape = new BMShapeTransform(definition, parent);
         shape->setType(BM_SHAPE_TRANS_IX);
         break;
     }
-    case BM_SHAPE_ELLIPSE_IX:
+    case BM_SHAPE_TAG('e', 'l'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse ellipse";
         shape = new BMEllipse(definition);
         shape->setType(BM_SHAPE_ELLIPSE_IX);
         break;
     }
-    case BM_SHAPE_ROUND_IX:
+    case BM_SHAPE_TAG('r', 'd'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse round";
         shape = new BMRound(definition, parent);
         shape->setType(BM_SHAPE_ROUND_IX);
         break;
     }
-    case BM_SHAPE_SHAPE_IX:
+    case BM_SHAPE_TAG('s', 'h'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse shape";
         shape = new BMFreeFormShape(definition, parent);
         shape->setType(BM_SHAPE_SHAPE_IX);
         break;
     }
-    case BM_SHAPE_TRIM_IX:
+    case BM_SHAPE_TAG('t', 'm'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse trim path";
         shape = new BMTrimPath(definition, parent);
         shape->setType(BM_SHAPE_TRIM_IX);
         break;
     }
-    case BM_SHAPE_REPEATER_IX:
+    case BM_SHAPE_TAG('r', 'p'):
     {
         qCDebug(lcLottieQtBodymovinParser) << "Parse trim path";
         shape = new BMRepeater(definition, parent);
         shape->setType(BM_SHAPE_REPEATER_IX);
         break;
     }
+    case BM_SHAPE_TAG('g', 's'): // ### BM_SHAPE_GSTROKE_IX
+    case BM_SHAPE_TAG('s', 'r'): // ### BM_SHAPE_STAR_IX
+        // fall through
     default:
         qCWarning(lcLottieQtBodymovinParser) << "Unsupported shape type:"
                                              << type;
     }
+
+#undef BM_SHAPE_TAG
+
     return shape;
 }
 
diff --git a/src/bodymovin/bmshape_p.h b/src/bodymovin/bmshape_p.h
index 349be5a..97caad6 100644
--- a/src/bodymovin/bmshape_p.h
+++ b/src/bodymovin/bmshape_p.h
@@ -41,7 +41,6 @@
 // We mean it.
 //
 
-#include <QLatin1String>
 #include <QPainterPath>
 
 #include <QtBodymovin/private/bmbase_p.h>
@@ -88,11 +87,6 @@ protected:
     QPainterPath m_path;
     BMTrimPath *m_appliedTrim = nullptr;
     int m_direction = 0;
-
-private:
-    static QMap<QLatin1String, int> setShapeMap();
-
-    static const QMap<QLatin1String, int> m_shapeMap;
 };
 
 QT_END_NAMESPACE
diff --git a/tests/auto/bodymovin/shape/shapetransform/tst_bmshapetransform.cpp b/tests/auto/bodymovin/shape/shapetransform/tst_bmshapetransform.cpp
index 4a5e78a..bd4ebb5 100644
--- a/tests/auto/bodymovin/shape/shapetransform/tst_bmshapetransform.cpp
+++ b/tests/auto/bodymovin/shape/shapetransform/tst_bmshapetransform.cpp
@@ -489,11 +489,16 @@ void tst_BMShapeTransform::loadTestData(const QByteArray &filename)
     BMGroup* group = nullptr;
     while (shapesIt != shapes.end()) {
         QJsonObject childObj = (*shapesIt).toObject();
-        QByteArray type = childObj.value(QLatin1String("ty")).toVariant().toByteArray();
-        if (QLatin1String(type.data()) == QLatin1String(BM_SHAPE_GROUP_STR))
-            group = new BMGroup(childObj);
+        BMShape *shape = BMShape::construct(childObj);
+        QVERIFY(shape != nullptr);
+        if (shape->type() == BM_SHAPE_GROUP_IX) {
+            group = static_cast<BMGroup *>(shape);
+            break;
+        }
         shapesIt++;
     }
+    QVERIFY(group != nullptr);
+
     m_transform = static_cast<BMShapeTransform*>(group->findChild("Transform"));
 
     QVERIFY(m_transform != nullptr);