qdoc: Fix bugs in resolving inheritance and finding overridden functions

A class node must have pointers to the class nodes of its base classes, but these
pointers might not exist when qdoc creates the class node for the class. They might
not exist until all the index files and include files have been parsewd. qdoc was
trying to resolve the base classes too early. This update lets qdoc wait until it
is known that everything has been built before attempting to resolve inheritance.

This update also delays finding the pointer to the function node for the overridden
function for a function marked "override" until the pointer is needed. Instead of
storing the pointer to the node, the qualification path to the function is stored
as a string, and the string is used to look up the overridden function when it is
needed, which is only when the \reimp command is processed during output.

The function that resolves the pointer to the overridden function was moved to the
function node class, where it makes more sense. The way a few qdoc warnings are
reported was also changed.

Change-Id: Ia54642d11242386ae75139065f481e5d30f79fb5
Reviewed-by: Topi Reiniö <topi.reinio@qt.io>

11 files changed, 153 insertions, 98 deletions

diff --git a/src/qdoc/clangcodeparser.cpp b/src/qdoc/clangcodeparser.cpp
index d04a1b46c..61a5fd639 100644
--- a/src/qdoc/clangcodeparser.cpp
+++ b/src/qdoc/clangcodeparser.cpp
@@ -179,6 +179,40 @@ QString functionName(CXCursor cursor)
 }
 
 /*!
+  Reconstruct the qualified path name of a function that is
+  being overridden.
+ */
+static QString reconstructQualifiedPathForCursor(CXCursor cur) {
+    QString path;
+    auto kind = clang_getCursorKind(cur);
+    while (!clang_isInvalid(kind) && kind != CXCursor_TranslationUnit) {
+        switch (kind) {
+        case CXCursor_Namespace:
+        case CXCursor_StructDecl:
+        case CXCursor_ClassDecl:
+        case CXCursor_UnionDecl:
+        case CXCursor_ClassTemplate:
+            path.prepend("::");
+            path.prepend(fromCXString(clang_getCursorSpelling(cur)));
+            break;
+        case CXCursor_FunctionDecl:
+        case CXCursor_FunctionTemplate:
+        case CXCursor_CXXMethod:
+        case CXCursor_Constructor:
+        case CXCursor_Destructor:
+        case CXCursor_ConversionFunction:
+            path = functionName(cur);
+            break;
+        default:
+            break;
+        }
+        cur = clang_getCursorSemanticParent(cur);
+        kind = clang_getCursorKind(cur);
+    }
+    return path;
+}
+
+/*!
   Find the node from the QDocDatabase \a qdb that corrseponds to the declaration
   represented by the cursor \a cur, if it exists.
  */
@@ -361,7 +395,6 @@ public:
                 isInteresting = allHeaders_.contains(fi.canonicalFilePath());
                 isInterestingCache_[file] = isInteresting;
             }
-
             if (isInteresting) {
                 return visitHeader(cur, loc);
             }
@@ -655,9 +688,10 @@ CXChildVisitResult ClangVisitor::visitHeader(CXCursor cursor, CXSourceLocation l
             unsigned int numOverridden = 0;
             clang_getOverriddenCursors(cursor, &overridden, &numOverridden);
             for (uint i = 0; i < numOverridden; ++i) {
-                auto n = findNodeForCursor(qdb_, overridden[i]);
-                if (n && n->isFunction()) {
-                    fn->setReimplementedFrom(static_cast<FunctionNode *>(n));
+                QString path = reconstructQualifiedPathForCursor(overridden[i]);
+                if (!path.isEmpty()) {
+                    fn->setReimplementedFrom(path);
+                    break;
                 }
             }
             clang_disposeOverriddenCursors(overridden);
@@ -1218,7 +1252,6 @@ void ClangCodeParser::buildPCH()
                 args_.pop_back(); // remove the "-xc++";
             }
         }
-        qdb_->resolveInheritance();
     }
 }
 
diff --git a/src/qdoc/codemarker.cpp b/src/qdoc/codemarker.cpp
index f655f28de..44d05053e 100644
--- a/src/qdoc/codemarker.cpp
+++ b/src/qdoc/codemarker.cpp
@@ -473,7 +473,7 @@ bool CodeMarker::insertReimpFunc(FastSection& fs, Node* node, Status status)
 {
     if ((node->access() != Node::Private) && (node->relates() == 0)) {
         const FunctionNode* fn = static_cast<const FunctionNode*>(node);
-        if ((fn->reimplementedFrom() != 0) && (status == Okay)) {
+        if (!fn->reimplementedFrom().isEmpty() && (status == Okay)) {
             if (fn->parent() == fs.parent_) {
                 QString key = sortName(fn);
                 if (!fs.reimpMemberMap.contains(key)) {
diff --git a/src/qdoc/cppcodeparser.cpp b/src/qdoc/cppcodeparser.cpp
index 9ac687557..9df4cc678 100644
--- a/src/qdoc/cppcodeparser.cpp
+++ b/src/qdoc/cppcodeparser.cpp
@@ -626,28 +626,12 @@ void CppCodeParser::processOtherMetaCommand(const Doc& doc,
         if (node != 0 && node->parent() && !node->parent()->isInternal()) {
             if (node->type() == Node::Function) {
                 FunctionNode *func = (FunctionNode *) node;
-                const FunctionNode *from = func->reimplementedFrom();
-                if (from == 0) {
-                    if (isWorthWarningAbout(doc)) {
-                        doc.location().warning(tr("Cannot find base function for '\\%1' in %2()")
-                                               .arg(COMMAND_REIMP).arg(node->name()),
-                                               tr("The function either doesn't exist in any "
-                                                  "base class with the same signature or it "
-                                                  "exists but isn't virtual."));
-                    }
-                }
-                /*
-                  Ideally, we would enable this check to warn whenever
-                  \reimp is used incorrectly, and only make the node
-                  internal if the function is a reimplementation of
-                  another function in a base class.
-                */
-                else if ((from->access() == Node::Private
-                          || from->parent()->access() == Node::Private)
-                         && isWorthWarningAbout(doc)) {
-                    doc.location().warning(tr("'\\%1' in %2() should be '\\internal' "
-                                              "because its base function is private "
-                                              "or internal").arg(COMMAND_REIMP).arg(node->name()));
+                if (func->reimplementedFrom().isEmpty() && isWorthWarningAbout(doc)) {
+                    doc.location().warning(tr("Cannot find base function for '\\%1' in %2()")
+                                           .arg(COMMAND_REIMP).arg(node->name()),
+                                           tr("The function either doesn't exist in any "
+                                              "base class with the same signature or it "
+                                              "exists but isn't virtual."));
                 }
                 func->setReimplemented(true);
             }
diff --git a/src/qdoc/generator.cpp b/src/qdoc/generator.cpp
index d822c8e64..1256477c2 100644
--- a/src/qdoc/generator.cpp
+++ b/src/qdoc/generator.cpp
@@ -868,7 +868,7 @@ void Generator::generateBody(const Node *node, CodeMarker *marker)
     else if (!node->isSharingComment()) {
         if (node->type() == Node::Function) {
             const FunctionNode *func = static_cast<const FunctionNode *>(node);
-            if (func->reimplementedFrom() != 0)
+            if (!func->reimplementedFrom().isEmpty())
                 generateReimplementedFrom(func, marker);
         }
 
@@ -1319,19 +1319,20 @@ bool Generator::generateQmlText(const Text& text,
     return result;
 }
 
-void Generator::generateReimplementedFrom(const FunctionNode *func,
-                                          CodeMarker *marker)
+void Generator::generateReimplementedFrom(const FunctionNode *fn, CodeMarker *marker)
 {
-    if (func->reimplementedFrom() != 0) {
-        const FunctionNode *from = func->reimplementedFrom();
-        if (from->access() != Node::Private &&
-                from->parent()->access() != Node::Private) {
-            Text text;
-            text << Atom::ParaLeft << "Reimplemented from ";
-            QString fullName =  from->parent()->name() + "::" + from->name() + "()";
-            appendFullName(text, from->parent(), fullName, from);
-            text << "." << Atom::ParaRight;
-            generateText(text, func, marker);
+    if (!fn->reimplementedFrom().isEmpty()) {
+        if (fn->parent()->isClass()) {
+            ClassNode* cn = static_cast<ClassNode*>(fn->parent());
+            const FunctionNode *from = cn->findOverriddenFunction(fn);
+            if (from && from->access() != Node::Private && from->parent()->access() != Node::Private) {
+                Text text;
+                text << Atom::ParaLeft << "Reimplemented from ";
+                QString fullName =  from->parent()->name() + "::" + from->name() + "()";
+                appendFullName(text, from->parent(), fullName, from);
+                text << "." << Atom::ParaRight;
+                generateText(text, fn, marker);
+            }
         }
     }
 }
diff --git a/src/qdoc/main.cpp b/src/qdoc/main.cpp
index 172261281..5232d3a24 100644
--- a/src/qdoc/main.cpp
+++ b/src/qdoc/main.cpp
@@ -471,8 +471,6 @@ static void processQdocconfFile(const QString &fileName)
         }
 
         clangParser_->precompileHeaders();
-        // Moved into ClangCodeParser after building PCH
-        //qdb->resolveInheritance();
 
         /*
           Parse each source text file in the set using the appropriate parser and
@@ -498,6 +496,7 @@ static void processQdocconfFile(const QString &fileName)
           targets, URLs, links, and other stuff that needs resolving.
         */
         qCDebug(lcQdoc, "Resolving stuff prior to generating docs");
+        qdb->resolveInheritance();
         qdb->resolveIssues();
     }
     else {
diff --git a/src/qdoc/node.cpp b/src/qdoc/node.cpp
index 9fe459690..e2ed08032 100644
--- a/src/qdoc/node.cpp
+++ b/src/qdoc/node.cpp
@@ -1814,6 +1814,35 @@ QmlTypeNode* ClassNode::findQmlBaseNode()
 }
 
 /*!
+  \a fn is an overriding function in this class or in a class
+  derived from this class. Find the node for the function that
+  \a fn overrides in this class's children or in one of this
+  class's base classes. Return a pointer to the overridden
+  function or return 0.
+ */
+FunctionNode* ClassNode::findOverriddenFunction(const FunctionNode* fn)
+{
+    QList<RelatedClass>::Iterator bc = bases_.begin();
+    while (bc != bases_.end()) {
+        ClassNode *cn = bc->node_;
+        if (!cn) {
+            cn = QDocDatabase::qdocDB()->findClassNode(bc->path_);
+            bc->node_ = cn;
+        }
+        if (cn) {
+            FunctionNode* result = cn->findFunctionNode(fn);
+            if (result && !result->isNonvirtual())
+                return result;
+            result = cn->findOverriddenFunction(fn);
+            if (result && !result->isNonvirtual())
+                return result;
+        }
+        ++bc;
+    }
+    return 0;
+}
+
+/*!
   \class DocumentNode
  */
 
@@ -2065,8 +2094,7 @@ FunctionNode::FunctionNode(Aggregate *parent, const QString& name)
       isImplicit_(false),
       isRef_(false),
       isRefRef_(false),
-      isInvokable_(false),
-      reimplementedFrom_(0)
+      isInvokable_(false)
 {
     setGenus(Node::CPP);
 }
@@ -2096,8 +2124,7 @@ FunctionNode::FunctionNode(NodeType type, Aggregate *parent, const QString& name
       isImplicit_(false),
       isRef_(false),
       isRefRef_(false),
-      isInvokable_(false),
-      reimplementedFrom_(0)
+      isInvokable_(false)
 {
     setGenus(Node::QML);
     if (type == QmlMethod || type == QmlSignal) {
diff --git a/src/qdoc/node.h b/src/qdoc/node.h
index 51f4b46ee..faea2dcb1 100644
--- a/src/qdoc/node.h
+++ b/src/qdoc/node.h
@@ -560,6 +560,7 @@ public:
     void setAbstract(bool b) override { abstract_ = b; }
     PropertyNode* findPropertyNode(const QString& name);
     QmlTypeNode* findQmlBaseNode();
+    FunctionNode* findOverriddenFunction(const FunctionNode* fn);
 
 private:
     QList<RelatedClass> bases_;
@@ -947,7 +948,7 @@ public:
     inline void setParameters(const QVector<Parameter>& parameters);
     void setParameters(const QString &t);
     void borrowParameterNames(const FunctionNode* source);
-    void setReimplementedFrom(FunctionNode* from) { reimplementedFrom_ = from; }
+    void setReimplementedFrom(const QString &path) { reimplementedFrom_ = path; }
 
     const QString& returnType() const { return returnType_; }
     QString metaness() const;
@@ -1000,7 +1001,7 @@ public:
     void clearParams() { parameters_.clear(); }
     QStringList parameterNames() const;
     QString rawParameters(bool names = false, bool values = false) const;
-    const FunctionNode* reimplementedFrom() const { return reimplementedFrom_; }
+    const QString& reimplementedFrom() const { return reimplementedFrom_; }
     const PropNodeList& associatedProperties() const { return associatedProperties_; }
     const QStringList& parentPath() const { return parentPath_; }
     bool hasAssociatedProperties() const { return !associatedProperties_.isEmpty(); }
@@ -1083,7 +1084,7 @@ private:
     bool isInvokable_ : 1;
     unsigned char overloadNumber_;
     QVector<Parameter> parameters_;
-    const FunctionNode* reimplementedFrom_;
+    QString             reimplementedFrom_;
     PropNodeList        associatedProperties_;
     QString             tag_;
 };
diff --git a/src/qdoc/qdocdatabase.cpp b/src/qdoc/qdocdatabase.cpp
index 55e916e74..8a4b276e8 100644
--- a/src/qdoc/qdocdatabase.cpp
+++ b/src/qdoc/qdocdatabase.cpp
@@ -373,6 +373,43 @@ QString QDocForest::getLinkCounts(QStringList& strings, QVector<int>& counts)
 }
 
 /*!
+  Finds the node for the qualified function path in \a target
+  that also has the parameters in \a params and returns it.
+  \a target is the path name to the function, without a return
+  type and withpout the parameters.
+
+  \a relative is the node in the primary tree where the search
+  begins. It is not used in the other trees, if the node is not
+  found in the primary tree. \a genus can be used to force the
+  search to find a C++ function or a QML function.
+
+  The entire forest is searched, but the first match is accepted.
+ */
+const FunctionNode* QDocForest::findFunctionNode(const QString& target,
+                                                 const QString& params,
+                                                 const Node* relative,
+                                                 Node::Genus genus)
+{
+    foreach (Tree* t, searchOrder()) {
+        const Node* n = t->findFunctionNode(target, params, relative, genus);
+        if (n)
+            return static_cast<const FunctionNode *>(n);
+        relative = 0;
+    }
+    return 0;
+}
+
+/*!
+  Finds the node for the qualified function path in \a target
+  and returns it. \a target is a complete function signature
+  without the return type.
+
+  \a relative is the node in the primary tree where the search
+  begins. It is not used in the other trees, if the node is not
+  found in the primary tree. \a genus can be used to force the
+  search to find a C++ function or a QML function.
+
+  The entire forest is searched, but the first match is accepted.
  */
 const Node* QDocForest::findFunctionNode(const QString& target,
                                          const Node* relative,
@@ -387,13 +424,7 @@ const Node* QDocForest::findFunctionNode(const QString& target,
     }
     else
         function = target;
-    foreach (Tree* t, searchOrder()) {
-        const Node* n = t->findFunctionNode(function, params, relative, genus);
-        if (n)
-            return n;
-        relative = 0;
-    }
-    return 0;
+    return findFunctionNode(function, params, relative, genus);
 }
 
 /*! \class QDocDatabase
diff --git a/src/qdoc/qdocdatabase.h b/src/qdoc/qdocdatabase.h
index 9b3e7628b..b2f4f02bc 100644
--- a/src/qdoc/qdocdatabase.h
+++ b/src/qdoc/qdocdatabase.h
@@ -137,7 +137,10 @@ class QDocForest
     const Node* findFunctionNode(const QString& target,
                                  const Node* relative,
                                  Node::Genus genus);
-
+    const FunctionNode* findFunctionNode(const QString& target,
+                                         const QString& params,
+                                         const Node* relative,
+                                         Node::Genus genus);
     const Node* findNodeForTarget(QStringList& targetPath,
                                   const Node* relative,
                                   Node::Genus genus,
@@ -337,6 +340,12 @@ class QDocDatabase
     const Node* findFunctionNode(const QString& target, const Node* relative, Node::Genus genus) {
         return forest_.findFunctionNode(target, relative, genus);
     }
+    const FunctionNode* findFunctionNode(const QString& target,
+                                         const QString& params,
+                                         const Node* relative,
+                                         Node::Genus genus) {
+        return forest_.findFunctionNode(target, params, relative, genus);
+    }
     const Node* findTypeNode(const QString& type, const Node* relative, Node::Genus genus);
     const Node* findNodeForTarget(const QString& target, const Node* relative);
     const DocumentNode* findDocumentNodeByTitle(const QString& title) {
diff --git a/src/qdoc/tree.cpp b/src/qdoc/tree.cpp
index 724e5ecb9..b51f3dc50 100644
--- a/src/qdoc/tree.cpp
+++ b/src/qdoc/tree.cpp
@@ -274,15 +274,16 @@ const FunctionNode* Tree::findFunctionNode(const QStringList& path,
             // functions are private.
             const FunctionNode* func = static_cast<const FunctionNode*>(node);
             while (func->access() == Node::Private) {
-                const FunctionNode* from = func->reimplementedFrom();
-                if (from != 0) {
-                    if (from->access() != Node::Private)
-                        return from;
-                    else
-                        func = from;
-                }
-                else
+                if (func->reimplementedFrom().isEmpty())
+                    return func;
+                QString path = func->reimplementedFrom();
+                const FunctionNode* from = qdb_->findFunctionNode(path, params, relative, genus);
+                if (from == 0)
                     break;
+                if (from->access() != Node::Private)
+                    return from;
+                else
+                    func = from;
             }
             return func;
         }
@@ -415,16 +416,7 @@ void Tree::resolveInheritanceHelper(int pass, ClassNode* cn)
     else {
         NodeList::ConstIterator c = cn->childNodes().constBegin();
         while (c != cn->childNodes().constEnd()) {
-            if ((*c)->type() == Node::Function) {
-                FunctionNode* func = (FunctionNode*)* c;
-                FunctionNode* from = findVirtualFunctionInBaseClasses(cn, func);
-                if (from != 0) {
-                    if (func->isNonvirtual())
-                        func->setVirtual();
-                    func->setReimplementedFrom(from);
-                }
-            }
-            else if ((*c)->type() == Node::Property)
+            if ((*c)->type() == Node::Property)
                 cn->fixPropertyUsingBaseClasses(static_cast<PropertyNode*>(*c));
             ++c;
         }
@@ -546,26 +538,6 @@ void Tree::fixInheritance(NamespaceNode* rootNode)
 
 /*!
  */
-FunctionNode* Tree::findVirtualFunctionInBaseClasses(ClassNode* cn, FunctionNode* clone)
-{
-    const QList<RelatedClass>& rc = cn->baseClasses();
-    QList<RelatedClass>::ConstIterator r = rc.constBegin();
-    while (r != rc.constEnd()) {
-        FunctionNode* func;
-        if ((*r).node_) {
-            if (((func = findVirtualFunctionInBaseClasses((*r).node_, clone)) != 0 ||
-                 (func = (*r).node_->findFunctionNode(clone)) != 0)) {
-                if (!func->isNonvirtual())
-                    return func;
-            }
-        }
-        ++r;
-    }
-    return 0;
-}
-
-/*!
- */
 NodeList Tree::allBaseClasses(const ClassNode* classNode) const
 {
     NodeList result;
diff --git a/src/qdoc/tree.h b/src/qdoc/tree.h
index 1aac6d664..136928fc1 100644
--- a/src/qdoc/tree.h
+++ b/src/qdoc/tree.h
@@ -168,8 +168,6 @@ class Tree
                                          Node::Genus genus = Node::DontCare) const;
     const NamespaceNode *root() const { return &root_; }
 
-    FunctionNode *findVirtualFunctionInBaseClasses(ClassNode *classe,
-                                                   FunctionNode *clone);
     NodeList allBaseClasses(const ClassNode *classe) const;
     QString refForAtom(const Atom* atom);