qmldom: improve Path

* PathEls::PathComponent allows heap free handling of a single path
  component, so make it more public and allow the creation and append to
  a Path using it.
* ensure initialization
* use QString in PathEls::Key (will need an extra allocation in
  fromString, but spares it on key lookups)
* make operator bool explicit (it is dangerous to potentially
  convert to int silently)
* consider top==env==universe simplifying path comparisons

Change-Id: I293674366a260b61cb7f5a65912714022aff218a
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

2 files changed, 133 insertions, 44 deletions

diff --git a/src/qmldom/qqmldompath.cpp b/src/qmldom/qqmldompath.cpp
index d119fa0e55..21bcf4c8b8 100644
--- a/src/qmldom/qqmldompath.cpp
+++ b/src/qmldom/qqmldompath.cpp
@@ -162,7 +162,13 @@ int PathComponent::cmp(const PathComponent &p1, const PathComponent &p2)
         return p1.data.key.keyValue.compare(p2.data.key.keyValue);
     case Kind::Root:
     {
-        int c = int(p1.data.root.contextKind) - int(p2.data.root.contextKind);
+        PathRoot k1 = p1.data.root.contextKind;
+        PathRoot k2 = p2.data.root.contextKind;
+        if (k1 == PathRoot::Env || k1 == PathRoot::Universe)
+            k1 = PathRoot::Top;
+        if (k2 == PathRoot::Env || k2 == PathRoot::Universe)
+            k2 = PathRoot::Top;
+        int c = int(k1) - int(k2);
         if (c != 0)
             return c;
         return p1.data.root.contextName.compare(p2.data.root.contextName);
@@ -398,12 +404,10 @@ Path Path::fromString(QStringView s, ErrorHandler errorHandler)
                 i0 = i-1;
                 while (i < s.length() && (s.at(i).isLetterOrNumber() || s.at(i) == underscore || s.at(i) == tilda))
                     ++i;
-                components.append(Component(PathEls::Key(s.mid(i0, i-i0))));
+                components.append(Component(PathEls::Key(s.mid(i0, i - i0).toString())));
             } else if (c == quote) {
                 i0 = i;
                 QString strVal;
-                QStringView key;
-                bool needsConversion = false;
                 bool properEnd = false;
                 while (i < s.length()) {
                     c = s.at(i);
@@ -411,7 +415,6 @@ Path Path::fromString(QStringView s, ErrorHandler errorHandler)
                         properEnd = true;
                         break;
                     } else if (c == backslash) {
-                        needsConversion = true;
                         strVal.append(s.mid(i0, i - i0).toString());
                         c = s.at(++i);
                         i0 = i + 1;
@@ -427,20 +430,14 @@ Path Path::fromString(QStringView s, ErrorHandler errorHandler)
                     ++i;
                 }
                 if (properEnd) {
-                    if (needsConversion) {
-                        strVal.append(s.mid(i0, i - i0).toString());
-                        strVals.append(strVal);
-                        key=strVal;
-                    } else {
-                        key = s.mid(i0, i - i0);
-                    }
+                    strVal.append(s.mid(i0, i - i0).toString());
                     ++i;
                 } else {
                     myErrors().error(tr("Unclosed quoted string at char %1.")
                                      .arg(QString::number(i - 1))).handle(errorHandler);
                     return Path();
                 }
-                components.append(PathEls::Key(key));
+                components.append(PathEls::Key(strVal));
             } else if (c == QChar::fromLatin1('*')) {
                 components.append(Component(PathEls::Any()));
             } else if (c == QChar::fromLatin1('?')) {
@@ -596,14 +593,17 @@ Path Path::Field(QString s)
 
 Path Path::Key(QStringView s)
 {
-    return Path(0,1,std::shared_ptr<PathEls::PathData>(
-                    new PathEls::PathData(QStringList(), QVector<Component>(1,Component(PathEls::Key(s))))));
+    return Path(
+            0, 1,
+            std::shared_ptr<PathEls::PathData>(new PathEls::PathData(
+                    QStringList(), QVector<Component>(1, Component(PathEls::Key(s.toString()))))));
 }
 
 Path Path::Key(QString s)
 {
-    return Path(0,1,std::shared_ptr<PathEls::PathData>(
-                    new PathEls::PathData(QStringList(s), QVector<Component>(1,Component(PathEls::Key(s))))));
+    return Path(0, 1,
+                std::shared_ptr<PathEls::PathData>(new PathEls::PathData(
+                        QStringList(), QVector<Component>(1, Component(PathEls::Key(s))))));
 }
 
 Path Path::Current(PathCurrent s)
@@ -654,19 +654,17 @@ Path Path::field(QStringView name) const
 
 Path Path::key(QString name) const
 {
-    auto res = key(QStringView(name));
-    res.m_data->strData.append(name);
-    return res;
-}
-
-Path Path::key(QStringView name) const
-{
     if (m_endOffset != 0)
         return noEndOffset().key(name);
     return Path(0,m_length+1,std::shared_ptr<PathEls::PathData>(
                     new PathEls::PathData(QStringList(), QVector<Component>(1,Component(PathEls::Key(name))), m_data)));
 }
 
+Path Path::key(QStringView name) const
+{
+    return key(name.toString());
+}
+
 Path Path::index(index_type i) const
 {
     if (m_endOffset != 0)
@@ -871,6 +869,66 @@ Path Path::noEndOffset() const
     return Path(0, m_length, lastData);
 }
 
+Path Path::appendComponent(const PathEls::PathComponent &c)
+{
+    if (m_endOffset != 0) {
+        Path newP = noEndOffset();
+        return newP.appendComponent(c);
+    }
+    if (m_data && m_data.use_count() != 1) {
+        // create a new path (otherwise paths linking to this will change)
+        Path newP(c);
+        newP.m_data->parent = m_data;
+        newP.m_length = static_cast<quint16>(m_length + 1);
+        return newP;
+    }
+    std::shared_ptr<PathEls::PathData> my_data =
+            (m_data ? m_data
+                    : std::make_shared<PathEls::PathData>(QStringList(),
+                                                          QVector<PathEls::PathComponent>()));
+    switch (c.kind()) {
+    case PathEls::Kind::Any:
+    case PathEls::Kind::Empty:
+    case PathEls::Kind::Index:
+        // no string
+    case PathEls::Kind::Field:
+        // string assumed to stay valid (Fields::...)
+        my_data->components.append(c);
+        break;
+    case PathEls::Kind::Current:
+        if (c.asCurrent()->contextKind == PathCurrent::Other) {
+            my_data->strData.append(c.asCurrent()->contextName.toString());
+            my_data->components.append(PathEls::Current(my_data->strData.last()));
+        } else {
+            my_data->components.append(c);
+        }
+        break;
+    case PathEls::Kind::Filter:
+        if (!c.base()->asFilter()->filterDescription.isEmpty()) {
+            my_data->strData.append(c.base()->asFilter()->filterDescription.toString());
+            my_data->components.append(
+                    PathEls::Filter(c.base()->asFilter()->filterFunction, my_data->strData.last()));
+        } else {
+            my_data->components.append(c);
+        }
+        break;
+    case PathEls::Kind::Key:
+        my_data->components.append(c);
+        break;
+    case PathEls::Kind::Root:
+        if (c.asRoot()->contextKind == PathRoot::Other) {
+            my_data->strData.append(c.asRoot()->contextName.toString());
+            my_data->components.append(PathEls::Root(my_data->strData.last()));
+        } else {
+            my_data->components.append(c);
+        }
+        break;
+    }
+    if (m_data)
+        m_endOffset = 1;
+    return Path { 0, static_cast<quint16>(m_length + 1), my_data };
+}
+
 ErrorGroups Path::myErrors()
 {
     static ErrorGroups res = {{NewErrorGroup("PathParsing")}};
diff --git a/src/qmldom/qqmldompath_p.h b/src/qmldom/qqmldompath_p.h
index 30ee9076b0..7d3d751183 100644
--- a/src/qmldom/qqmldompath_p.h
+++ b/src/qmldom/qqmldompath_p.h
@@ -126,6 +126,7 @@ public:
 
 class Empty: public Base {
 public:
+    Empty() = default;
     Kind kind() const override { return Kind::Empty; }
     QString name() const override { return QString(); }
     bool checkName(QStringView s) const override { return s.isEmpty(); }
@@ -134,6 +135,7 @@ public:
 
 class Field: public Base {
 public:
+    Field() = default;
     Field(QStringView n): fieldName(n) {}
     Kind kind() const override { return Kind::Field; }
     QString name() const override { return fieldName.toString(); }
@@ -147,6 +149,7 @@ public:
 
 class Index: public Base {
 public:
+    Index() = default;
     Index(index_type i): indexValue(i) {}
     Kind kind() const override { return Kind::Index; }
     QString name() const override { return QString::number(indexValue); }
@@ -155,14 +158,15 @@ public:
     bool hasSquareBrackets() const override { return true; }
     const Index * asIndex() const override { return this; }
 
-    index_type indexValue;
+    index_type indexValue = -1;
 };
 
 class Key: public Base {
 public:
-    Key(QStringView n): keyValue(n) {}
+    Key() = default;
+    Key(QString n) : keyValue(n) { }
     Kind kind() const override { return Kind::Key; }
-    QString name() const override { return keyValue.toString(); }
+    QString name() const override { return keyValue; }
     bool checkName(QStringView s) const override { return s == keyValue; }
     QStringView stringView() const override { return keyValue; }
     void dump(Sink sink) const override {
@@ -173,12 +177,12 @@ public:
     bool hasSquareBrackets() const override { return true; }
     const Key * asKey() const override { return this; }
 
-    QStringView keyValue;
+    QString keyValue;
 };
 
 class Root: public Base {
 public:
-    Root(): contextKind(PathRoot::Other), contextName() {}
+    Root() = default;
     Root(PathRoot r): contextKind(r), contextName() {}
     Root(QStringView n) {
         QMetaEnum metaEnum = QMetaEnum::fromType<PathRoot>();
@@ -221,13 +225,13 @@ public:
     }
     const Root *asRoot() const override { return this; }
 
-    PathRoot contextKind;
+    PathRoot contextKind = PathRoot::Other;
     QStringView contextName;
 };
 
 class Current: public Base {
 public:
-    Current(): contextName() {}
+    Current() = default;
     Current(PathCurrent c): contextKind(c) {}
     Current(QStringView n) {
         QMetaEnum metaEnum = QMetaEnum::fromType<PathCurrent>();
@@ -275,12 +279,13 @@ public:
     QStringView stringView() const override { return contextName; }
     const Current *asCurrent() const override { return this; }
 
-    PathCurrent contextKind;
+    PathCurrent contextKind = PathCurrent::Other;
     QStringView contextName;
 };
 
 class Any: public Base {
 public:
+    Any() = default;
     Kind kind() const override { return Kind::Any; }
     QString name() const override { return QLatin1String("*"); }
     bool checkName(QStringView s) const override { return s == u"*"; }
@@ -290,6 +295,7 @@ public:
 
 class QMLDOM_EXPORT Filter: public Base {
 public:
+    Filter() = default;
     Filter(std::function<bool(DomItem)> f, QStringView filterDescription = u"<native code filter>");
     Kind kind() const override { return Kind::Filter; }
     QString name() const override;
@@ -323,9 +329,6 @@ public:
     const Current *asCurrent() const { return base()->asCurrent(); }
     const Any *asAny() const { return base()->asAny(); }
     static int cmp(const PathComponent &p1, const PathComponent &p2);
-private:
-    friend class QQmlJS::Dom::Path;
-    friend class QQmlJS::Dom::PathEls::TestPaths;
 
     PathComponent(const Empty &o): data(o) {}
     PathComponent(const Field &o): data(o) {}
@@ -336,6 +339,10 @@ private:
     PathComponent(const Any &o): data(o) {}
     PathComponent(const Filter &o): data(o) {}
 
+private:
+    friend class QQmlJS::Dom::Path;
+    friend class QQmlJS::Dom::PathEls::TestPaths;
+
     Base *base() {
         return reinterpret_cast<Base*>(&data);
     }
@@ -635,10 +642,14 @@ public:
     static ErrorGroups myErrors(); // use static consts and central registration instead?
 
     Path() = default;
+    explicit Path(const PathEls::PathComponent &c) : m_endOffset(0), m_length(0)
+    {
+        *this = appendComponent(c);
+    }
 
     int length() const { return m_length; }
     Path operator[](int i) const;
-    operator bool() const;
+    explicit operator bool() const;
 
     PathIterator begin() const;
     PathIterator end() const;
@@ -660,6 +671,7 @@ public:
     Path dropTail(int n = 1) const;
     Path mid(int offset, int length) const;
     Path mid(int offset) const;
+    Path appendComponent(const PathEls::PathComponent &c);
 
     // # Path construction
     static Path fromString(QString s, ErrorHandler errorHandler=nullptr);
@@ -705,12 +717,13 @@ public:
     using iterator_category = std::forward_iterator_tag;
 
     static int cmp(const Path &p1, const Path &p2);
+    Component component(int i) const;
+
 private:
     explicit Path(quint16 endOffset, quint16 length, std::shared_ptr<PathEls::PathData> data);
     friend class QQmlJS::Dom::PathEls::TestPaths;
     friend size_t qHash(const Path &, size_t);
 
-    Component component(int i) const;
     Path noEndOffset() const;
 
     quint16 m_endOffset = 0;
@@ -718,6 +731,31 @@ private:
     std::shared_ptr<PathEls::PathData> m_data = {};
 };
 
+inline bool operator==(const Path &lhs, const Path &rhs)
+{
+    return lhs.length() == rhs.length() && Path::cmp(lhs, rhs) == 0;
+}
+inline bool operator!=(const Path &lhs, const Path &rhs)
+{
+    return lhs.length() != rhs.length() || Path::cmp(lhs, rhs) != 0;
+}
+inline bool operator<(const Path &lhs, const Path &rhs)
+{
+    return Path::cmp(lhs, rhs) < 0;
+}
+inline bool operator>(const Path &lhs, const Path &rhs)
+{
+    return Path::cmp(lhs, rhs) > 0;
+}
+inline bool operator<=(const Path &lhs, const Path &rhs)
+{
+    return Path::cmp(lhs, rhs) <= 0;
+}
+inline bool operator>=(const Path &lhs, const Path &rhs)
+{
+    return Path::cmp(lhs, rhs) >= 0;
+}
+
 class PathIterator {
 public:
     Path currentEl;
@@ -728,13 +766,6 @@ public:
     bool operator !=(const PathIterator &o) { return currentEl != o.currentEl; }
 };
 
-inline bool operator==(const Path& lhs, const Path& rhs){ return lhs.length() == rhs.length() && Path::cmp(lhs,rhs) == 0; }
-inline bool operator!=(const Path& lhs, const Path& rhs){ return lhs.length() != rhs.length() || Path::cmp(lhs,rhs) != 0; }
-inline bool operator< (const Path& lhs, const Path& rhs){ return Path::cmp(lhs,rhs) <  0; }
-inline bool operator> (const Path& lhs, const Path& rhs){ return Path::cmp(lhs,rhs) >  0; }
-inline bool operator<=(const Path& lhs, const Path& rhs){ return Path::cmp(lhs,rhs) <= 0; }
-inline bool operator>=(const Path& lhs, const Path& rhs){ return Path::cmp(lhs,rhs) >= 0; }
-
 class Source {
 public:
     Path pathToSource;