bookmaker error handling

Bookmaker will generate instructions on how to
fix detected errors in a few cases:
- if class function is missing description
- if global function is missing description
- if function parameters don't match doxygen
- if function parameters don't match bmh

(The last case above won't happen if bmh #Method
uses #Populate to retrieve parameter descriptions
from the include.)

Adding this revealed that globals weren't always
accounted for in bookmaker's cross-check; fix
that as well.

TBR=reed@google.com
Docs-Preview: https://skia.org/?cl=171224
Bug: skia:
Change-Id: Ic1b41d4722954fa8a42685a8fe7266b8a860c362
Reviewed-on: https://skia-review.googlesource.com/c/171224
Reviewed-by: Cary Clark <caryclark@skia.org>
Commit-Queue: Cary Clark <caryclark@skia.org>
Auto-Submit: Cary Clark <caryclark@skia.org>

docs/SkColor_Reference.bmh

@@ -597,7 +597,7 @@ hsv[2] contains HSV_Value, a value from zero to one.
 
 # ------------------------------------------------------------------------------
 
-#Method void SkColorToHSV(SkColor color, SkScalar hsv[3])
+#Method static void SkColorToHSV(SkColor color, SkScalar hsv[3])
 #In Functions
 #Line # converts RGB to HSV ##
 
@@ -673,7 +673,7 @@ Out of range hsv values are pinned.
 
 # ------------------------------------------------------------------------------
 
-#Method SkColor SkHSVToColor(const SkScalar hsv[3])
+#Method static SkColor SkHSVToColor(const SkScalar hsv[3])
 #In Functions
 #Line # converts HSV to RGB ##
 

docs/SkImageInfo_Reference.bmh

@@ -189,7 +189,7 @@ in the same order.
 
 #Subtopic Unpremul ##
 
-#Method static inline bool SkAlphaTypeIsOpaque(SkAlphaType at)
+#Method static bool SkAlphaTypeIsOpaque(SkAlphaType at)
 #In Property
 #Line # returns if Alpha_Type equals kOpaque_SkAlphaType ##
 #Populate

docs/SkRegion_Reference.bmh

@@ -269,7 +269,7 @@ rect={1,2,3,4}
 #Line # iterator returning IRect within clip ##
 
 #Code
-    class SK_API Cliperator {
+    class Cliperator {
     public:
         Cliperator(const SkRegion& region, const SkIRect& clip);
         bool done();

docs/undocumented.bmh

@@ -27,7 +27,6 @@
  SkXXX_Reference # ditto
  Skia           # ditto
  SK_ABORT       # ditto
- SK_API         # ditto
  SK_DEBUG       # ditto
  SK_RELEASE     # ditto
  SK_USE_FREETYPE_EMBOLDEN # ditto
@@ -189,7 +188,7 @@ FT_Load_Glyph
 ##
 
 #Topic Create_Color_Space_Xform_Canvas
-#Method std::unique_ptr<SkCanvas> SK_API SkCreateColorSpaceXformCanvas(SkCanvas* target,
+#Method std::unique_ptr<SkCanvas> SkCreateColorSpaceXformCanvas(SkCanvas* target,
                                                                 sk_sp<SkColorSpace> targetCS)
 ##
 ##
@@ -217,7 +216,7 @@ FT_Load_Glyph
 ##
 
 #Topic Debugging
-#Method SK_API void SkDebugf(const char format[], ...)
+#Method void SkDebugf(const char format[], ...)
 ##
 ##
 
@@ -772,7 +771,7 @@ FT_Load_Glyph
 #Topic PathOps
     #Enum SkPathOp
     ##
-    #Method bool SK_API Op(const SkPath& one, const SkPath& two, SkPathOp op, SkPath* result)
+    #Method bool Op(const SkPath& one, const SkPath& two, SkPathOp op, SkPath* result)
     ##
 #Topic ##
 

site/user/api/SkColor_Reference.md

@@ -569,7 +569,7 @@ Converts RGB to its HSV components.
 ---
 
 <pre style="padding: 1em 1em 1em 1em; width: 62.5em;background-color: #f0f0f0">
-void <a href='SkColor_Reference#SkColorToHSV'>SkColorToHSV</a>(<a href='SkColor_Reference#SkColor'>SkColor</a> <a href='SkColor_Reference#Color'>color</a>, <a href='undocumented#SkScalar'>SkScalar</a> hsv[3])
+static void <a href='SkColor_Reference#SkColorToHSV'>SkColorToHSV</a>(<a href='SkColor_Reference#SkColor'>SkColor</a> <a href='SkColor_Reference#Color'>color</a>, <a href='undocumented#SkScalar'>SkScalar</a> hsv[3])
 </pre>
 
 Converts ARGB to its HSV components. <a href='SkColor_Reference#Alpha'>Alpha</a> in ARGB is ignored.

site/user/api/SkRegion_Reference.md

@@ -389,7 +389,7 @@ iterated <a href='SkRegion_Reference#SkRegion'>SkRegion</a>
 ---
 
 <pre style="padding: 1em 1em 1em 1em;width: 62.5em; background-color: #f0f0f0">
-    class SK_API <a href='#SkRegion_Cliperator'>Cliperator</a> {
+    class <a href='#SkRegion_Cliperator'>Cliperator</a> {
     public:
         <a href='#SkRegion_Cliperator'>Cliperator</a>(const <a href='SkRegion_Reference#SkRegion'>SkRegion</a>& <a href='SkRegion_Reference#Region'>region</a>, const <a href='SkIRect_Reference#SkIRect'>SkIRect</a>& clip);
         bool <a href='#SkRegion_Cliperator_done'>done()</a>;

tools/bookmaker/definition.cpp

@@ -185,12 +185,9 @@ bool Definition::parseOperator(size_t doubleColons, string& result) {
     string className(fName, 0, doubleColons - 2);
     TextParser iParser(fFileName, fStart, fContentStart, fLineCount);
     SkAssertResult(iParser.skipWord("#Method"));
-    iParser.skipExact("SK_API");
     iParser.skipWhiteSpace();
     bool isStatic = iParser.skipExact("static");
     iParser.skipWhiteSpace();
-    iParser.skipExact("SK_API");
-    iParser.skipWhiteSpace();
     bool returnsConst = iParser.skipExact("const");
     if (returnsConst) {
         SkASSERT(0);  // incomplete
@@ -644,11 +641,7 @@ const char* Definition::methodEnd() const {
     return tokenEnd;
 }
 
-bool Definition::crossCheckInside(const char* start, const char* end,
-        const Definition& includeToken) const {
-    TextParser def(fFileName, start, end, fLineCount);
-    const char* tokenEnd = includeToken.methodEnd();
-    TextParser inc("", includeToken.fContentStart, tokenEnd, 0);
+bool Definition::SkipImplementationWords(TextParser& inc) {
     if (inc.startsWith("SK_API")) {
         inc.skipWord("SK_API");
     }
@@ -661,7 +654,23 @@ bool Definition::crossCheckInside(const char* start, const char* end,
     if (inc.startsWith("SK_API")) {
         inc.skipWord("SK_API");
     }
-    inc.skipExact("SkDEBUGCODE(");
+    return inc.skipExact("SkDEBUGCODE(");
+}
+
+bool Definition::crossCheckInside(const char* start, const char* end,
+        const Definition& includeToken) const {
+    TextParser def(fFileName, start, end, fLineCount);
+    const char* tokenEnd = includeToken.methodEnd();
+    TextParser inc("", includeToken.fContentStart, tokenEnd, 0);
+    if (inc.startsWith("static")) {
+        def.skipWhiteSpace();
+        if (!def.startsWith("static")) {
+            return false;
+        }
+        inc.skipWord("static");
+        def.skipWord("static");
+    }
+    (void) Definition::SkipImplementationWords(inc);
     do {
         bool defEof;
         bool incEof;
@@ -1196,10 +1205,6 @@ bool RootDefinition::dumpUnVisited() {
             if ("SkBitmap::validate()" == leaf.first) {
                 continue;
             }
-            // SkPath::pathRefIsValid in #ifdef ; prefer to remove chrome dependency to fix
-            if ("SkPath::pathRefIsValid" == leaf.first) {
-                continue;
-            }
             // FIXME: end of long tail bugs
             SkDebugf("defined in bmh but missing in include: %s\n", leaf.first.c_str());
             success = false;

tools/bookmaker/definition.h

@@ -197,6 +197,8 @@ public:
         fParentIndex = fParent ? (int) fParent->fTokens.size() : -1;
     }
 
+    static bool SkipImplementationWords(TextParser& inc);
+
     string simpleName() {
         size_t doubleColon = fName.rfind("::");
         return string::npos == doubleColon ? fName : fName.substr(doubleColon + 2);

tools/bookmaker/includeParser.cpp

@@ -634,59 +634,41 @@ void IncludeParser::writeCodeBlock() {
 #include <sstream>
 #include <iostream>
 
-bool IncludeParser::crossCheck(BmhParser& bmhParser) {
-    for (auto& classMapper : fIClassMap) {
-        string className = classMapper.first;
-        auto finder = bmhParser.fClassMap.find(className);
-        if (bmhParser.fClassMap.end() == finder) {
-            SkASSERT(string::npos != className.find("::"));
-            continue;
-        }
-    }
-    for (auto& classMapper : fIClassMap) {
-        if (classMapper.second.fUndocumented) {
-           continue;
-        }
-        string className = classMapper.first;
-        std::istringstream iss(className);
-        string classStr;
-        string classBase;
-        RootDefinition* root = nullptr;
-        while (std::getline(iss, classStr, ':')) {
-            if (root) {
-                if (!classStr.length()) {
-                    continue;
-                }
-                classBase += "::" + classStr;
-                auto finder = root->fBranches.find(classBase);
-                if (root->fBranches.end() != finder) {
-                    root = finder->second;
-                } else {
-                    SkASSERT(0);
-                }
-            } else {
-                classBase = classStr;
-                auto finder = bmhParser.fClassMap.find(classBase);
-                if (bmhParser.fClassMap.end() != finder) {
-                    root = &finder->second;
-                } else {
-                    SkASSERT(0);
-                }
-            }
-        }
-        auto& classMap = classMapper.second;
-        auto& tokens = classMap.fTokens;
+void IncludeParser::checkTokens(list<Definition>& tokens, string key, string className,
+        RootDefinition* root, BmhParser& bmhParser) {
     for (const auto& token : tokens) {
         if (token.fPrivate) {
             continue;
         }
-            string fullName = classMapper.first + "::" + token.fName;
-            const Definition* def = root->find(fullName, RootDefinition::AllowParens::kYes);
+        string fullName = key + "::" + token.fName;
+        const Definition* def = nullptr;
+        if (root) {
+            def = root->find(fullName, RootDefinition::AllowParens::kYes);
+        }
         switch (token.fMarkType) {
             case MarkType::kMethod: {
                 if (this->isInternalName(token)) {
                     continue;
                 }
+                if (!root) {
+                    if (token.fUndocumented) {
+                        break;
+                    }
+                    auto methIter = bmhParser.fMethodMap.find(token.fName);
+                    if (bmhParser.fMethodMap.end() != methIter) {
+                        def = &methIter->second;
+                        if (def->crossCheck2(token)) {
+                            def->fVisited = true;
+                        } else {
+                            this->suggestFix(Suggest::kMethodDiffers, token, root, def);
+                            fFailed = true;
+                        }
+                    } else {
+                        this->suggestFix(Suggest::kMethodMissing, token, root, nullptr);
+                        fFailed = true;
+                    }
+                    break;
+                }
                 if (!def) {
                     string paramName = className + "::";
                     paramName += string(token.fContentStart,
@@ -773,12 +755,15 @@ bool IncludeParser::crossCheck(BmhParser& bmhParser) {
                 }
                 if (!def) {
                     if (!token.fUndocumented) {
-                            SkDebugf("method missing from bmh: %s\n", fullName.c_str());
+                        this->suggestFix(Suggest::kMethodMissing, token, root, nullptr);
                         fFailed = true;
                     }
                     break;
                 }
                 if (token.fUndocumented) {
+                    // we can't report an error yet; if bmh documents this unnecessarily,
+                    // we'll detect that later. It may be that def points to similar
+                    // documented function.
                     break;
                 }
                 if (def->crossCheck2(token)) {
@@ -816,9 +801,29 @@ bool IncludeParser::crossCheck(BmhParser& bmhParser) {
                     } while (firstMember.next());
                     if (lastUnderscore) {
                         ++lastUnderscore;
-                            string anonName = className + "::" + string(lastUnderscore,
-                                    wordEnd - lastUnderscore) + 's';
+                        string enumName(lastUnderscore, wordEnd - lastUnderscore);
+                        if (root) {
+                            string anonName = className + "::" + enumName + 's';
                             def = root->find(anonName, RootDefinition::AllowParens::kYes);
+                        } else {
+                            auto enumIter = bmhParser.fEnumMap.find(enumName);
+                            if (bmhParser.fEnumMap.end() != enumIter) {
+                                RootDefinition* rootDef = &enumIter->second;
+                                def = rootDef;
+                            }
+                        }
+                    }
+                    if (!def && !root) {
+                        auto enumIter = bmhParser.fEnumMap.find(token.fName);
+                        if (bmhParser.fEnumMap.end() != enumIter) {
+                            def = &enumIter->second;
+                        }
+                        if (!def) {
+                            auto enumClassIter = bmhParser.fClassMap.find(token.fName);
+                            if (bmhParser.fClassMap.end() != enumClassIter) {
+                                def = &enumClassIter->second;
+                            }
+                        }
                     }
                     if (!def) {
                         if (!token.fUndocumented) {
@@ -843,6 +848,13 @@ bool IncludeParser::crossCheck(BmhParser& bmhParser) {
                         break;
                     }
                 }
+                if (!hasCode && !root) {
+                    const Definition* topic = def->topicParent();
+                    hasCode = std::any_of(topic->fChildren.begin(), topic->fChildren.end(),
+                            [](Definition* def){ return MarkType::kCode == def->fMarkType
+                            && def->fChildren.size() > 0 && MarkType::kPopulate ==
+                            def->fChildren.front()->fMarkType; });
+                }
                 if (!hasCode) {
                     SkDebugf("enum code missing from bmh: %s\n", fullName.c_str());
                     fFailed = true;
@@ -862,10 +874,21 @@ bool IncludeParser::crossCheck(BmhParser& bmhParser) {
                     }
                     string constName = MarkType::kEnumClass == token.fMarkType ?
                             fullName : className;
+                    if (root) {
                         constName += "::" + member.fName;
                         def = root->find(constName, RootDefinition::AllowParens::kYes);
+                    } else {
+                        auto enumMapper = bmhParser.fEnumMap.find(token.fName);
+                        if (bmhParser.fEnumMap.end() != enumMapper) {
+                            auto& enumDoc = enumMapper->second;
+                            auto memberIter = enumDoc.fLeaves.find(member.fName);
+                            if (enumDoc.fLeaves.end() != memberIter) {
+                                def = &memberIter->second;
+                            }
+                        }
+                    }
                     if (!def) {
-                            string innerName = classMapper.first + "::" + member.fName;
+                        string innerName = key + "::" + member.fName;
                         def = root->find(innerName, RootDefinition::AllowParens::kYes);
                     }
                     if (!def) {
@@ -887,6 +910,12 @@ bool IncludeParser::crossCheck(BmhParser& bmhParser) {
                 }
                 break;
             case MarkType::kTypedef:
+                if (!def && !root) {
+                    auto typedefIter = bmhParser.fTypedefMap.find(token.fName);
+                    if (bmhParser.fTypedefMap.end() != typedefIter) {
+                        def = &typedefIter->second;
+                    }
+                }
                 if (def) {
                     def->fVisited = true;
                 } else {
@@ -895,6 +924,12 @@ bool IncludeParser::crossCheck(BmhParser& bmhParser) {
                 }
                 break;
             case MarkType::kConst:
+                if (!def && !root) {
+                    auto constIter = bmhParser.fConstMap.find(token.fName);
+                    if (bmhParser.fConstMap.end() != constIter) {
+                        def = &constIter->second;
+                    }
+                }
                 if (def) {
                     def->fVisited = true;
                 } else {
@@ -904,12 +939,60 @@ bool IncludeParser::crossCheck(BmhParser& bmhParser) {
                     }
                 }
                 break;
+            case MarkType::kDefine:
+                // TODO: incomplete
+                break;
             default:
                 SkASSERT(0);  // unhandled
                 break;
         }
     }
+}
+
+bool IncludeParser::crossCheck(BmhParser& bmhParser) {
+    for (auto& classMapper : fIClassMap) {
+        string className = classMapper.first;
+        auto finder = bmhParser.fClassMap.find(className);
+        if (bmhParser.fClassMap.end() == finder) {
+            SkASSERT(string::npos != className.find("::"));
+            continue;
         }
+    }
+    for (auto& classMapper : fIClassMap) {
+        if (classMapper.second.fUndocumented) {
+           continue;
+        }
+        string className = classMapper.first;
+        std::istringstream iss(className);
+        string classStr;
+        string classBase;
+        RootDefinition* root = nullptr;
+        while (std::getline(iss, classStr, ':')) {
+            if (root) {
+                if (!classStr.length()) {
+                    continue;
+                }
+                classBase += "::" + classStr;
+                auto finder = root->fBranches.find(classBase);
+                if (root->fBranches.end() != finder) {
+                    root = finder->second;
+                } else {
+                    SkASSERT(0);
+                }
+            } else {
+                classBase = classStr;
+                auto finder = bmhParser.fClassMap.find(classBase);
+                if (bmhParser.fClassMap.end() != finder) {
+                    root = &finder->second;
+                } else {
+                    SkASSERT(0);
+                }
+            }
+        }
+        this->checkTokens(classMapper.second.fTokens, classMapper.first, className, root,
+                bmhParser);
+    }
+    this->checkTokens(fGlobals, "", "", nullptr, bmhParser);
     int crossChecks = 0;
     string firstCheck;
     for (auto& classMapper : fIClassMap) {
@@ -3578,6 +3661,132 @@ void IncludeParser::RemoveOneFile(const char* docs, const char* includesFile) {
     remove(fullName.c_str());
 }
 
+static const char kMethodMissingStr[] =
+    "If the method requires documentation, add to "
+    "%s at minimum:\n"  // path to bmh file
+    "\n"
+    "#Method %s\n" // method declaration less implementation details
+    "#In  SomeSubtopicName\n"
+    "#Line # add a one line description here ##\n"
+    "#Populate\n"
+    "#NoExample\n"
+    "// or better yet, use #Example and put C++ code here\n"
+    "##\n"
+    "#SeeAlso optional related symbols\n"
+    "#Method ##\n"
+    "\n"
+    "Add to %s, at minimum:\n"  // path to include
+    "\n"
+    "/** (description) Starts with present tense action verb\n"
+    "    and end with a period.\n"
+    "%s"   // @param, @return if needed go here
+    "*/\n"
+    "%s ...\n" // method declaration
+    "\n"
+    "If the method does not require documentation,\n"
+    "add \"private\" or \"experimental\", as in:\n"
+    "\n"
+    "/** Experimental, do not use. And so on...\n"
+    "*/\n"
+    "%s ...\n" // method declaration
+    "\n"
+    ;
+
+// bDef does not have #Populate
+static const char kMethodDiffersNoPopStr[] =
+    "In %s:\n"              // path to bmh file
+    "#Method %s\n"          // method declaration less implementation details
+    "does not match doxygen comment of:\n"
+    "%s.\n"                 // method declaration
+    "\n"
+    ;
+
+static const char kMethodDiffersStr[] =
+    "In %s:\n"                        // path to include
+    "%s\n"                            // method declaration
+    "does not match doxygen comment.\n"
+    "\n"
+    ;
+
+void IncludeParser::suggestFix(Suggest suggest, const Definition& iDef,
+        const RootDefinition* root, const Definition* bDef) {
+    string methodNameStr(iDef.fContentStart, iDef.length());
+    const char* methodName = methodNameStr.c_str();
+    TextParser lessImplParser(&iDef);
+    if (lessImplParser.skipExact("static")) {
+        lessImplParser.skipWhiteSpace();
+    }
+    // TODO : handle debug wrapper
+    /* bool inDebugWrapper = */ Definition::SkipImplementationWords(lessImplParser);
+    string lessImplStr(lessImplParser.fChar, lessImplParser.fEnd - lessImplParser.fChar);
+    const char* methodNameLessImpl = lessImplStr.c_str();
+    // return result, if any is substr from 0 to location of iDef.fName
+    size_t namePos = methodNameStr.find(iDef.fName);
+    SkASSERT(string::npos != namePos);
+    size_t funcEnd = namePos;
+    while (funcEnd > 0 && ' ' >= methodNameStr[funcEnd - 1]) {
+        funcEnd -= 1;
+    }
+    string funcRet = methodNameStr.substr(0, funcEnd);
+// parameters, if any, are delimited by () and separate by ,
+    TextParser parser(&iDef);
+    parser.fChar += namePos + iDef.fName.length();
+    const char* start = parser.fChar;
+    vector<string> paramStrs;
+    if ('(' == start[0]) {
+        parser.skipToBalancedEndBracket('(', ')');
+        TextParser params(&iDef);
+        params.fChar = start + 1;
+        params.fEnd = parser.fChar;
+        while (!params.eof()) {
+            const char* paramEnd = params.anyOf("=,)");
+            const char* paramStart = paramEnd;
+            while (paramStart > params.fChar && ' ' >= paramStart[-1]) {
+                paramStart -= 1;
+            }
+            while (paramStart > params.fChar && (isalnum(paramStart[-1])
+                    || '_' == paramStart[-1])) {
+                paramStart -= 1;
+            }
+            string param(paramStart, paramEnd - paramStart);
+            paramStrs.push_back(param);
+            params.fChar = params.anyOf(",)") + 1;
+        }
+    }
+    string bmhFile = root ? root->fFileName : bDef ? bDef->fFileName : "a *.bmh file";
+    bool hasFuncReturn = "" != funcRet && "void" != funcRet;
+    switch(suggest) {
+        case Suggest::kMethodMissing: {
+            // if include @param, @return are missing, request them as well
+            string paramDox;
+            bool firstParam = true;
+            for (auto paramStr : paramStrs) {
+                if (firstParam) {
+                    paramDox += "\n";
+                    firstParam = false;
+                }
+                paramDox += "    @param " + paramStr + "  descriptive phrase\n";
+            }
+            if (hasFuncReturn) {
+                paramDox += "\n";
+                paramDox += "    @return descriptive phrase\n";
+            }
+            SkDebugf(kMethodMissingStr, bmhFile.c_str(), methodNameLessImpl, iDef.fFileName.c_str(),
+                    paramDox.c_str(), methodName, methodName);
+            } break;
+        case Suggest::kMethodDiffers: {
+            bool hasPop = std::any_of(bDef->fChildren.begin(), bDef->fChildren.end(),
+                    [](Definition* def) { return MarkType::kPopulate == def->fMarkType; });
+            if (!hasPop) {
+                SkDebugf(kMethodDiffersNoPopStr, bmhFile.c_str(), methodNameLessImpl, methodName);
+            }
+            SkDebugf(kMethodDiffersStr, iDef.fFileName.c_str(), methodName);
+            } break;
+        default:
+            SkASSERT(0);
+    }
+}
+
 Bracket IncludeParser::topBracket() const {
     Definition* parent = this->parentBracket(fParent);
     return parent ? parent->fBracket : Bracket::kNone;

tools/bookmaker/includeParser.h

@@ -36,6 +36,11 @@ public:
         kYes,
     };
 
+    enum class Suggest {
+        kMethodMissing,
+        kMethodDiffers,
+    };
+
     struct CheckCode {
         enum class State {
             kNone,
@@ -107,6 +112,8 @@ public:
     void checkForMissingParams(const vector<string>& methodParams,
                                const vector<string>& foundParams);
     bool checkForWord();
+    void checkTokens(list<Definition>& tokens, string key, string className,
+            RootDefinition* root, BmhParser& bmhParser);
     string className() const;
 
     string codeBlock(const Definition& def, bool inProgress) const {
@@ -261,6 +268,8 @@ public:
         fInCharCommentString = fInChar || fInComment || fInString;
     }
 
+    void suggestFix(Suggest suggest, const Definition& iDef, const RootDefinition* root,
+            const Definition* bDef);
     Bracket topBracket() const;
 
     template <typename T>