fix self references

try removing self references in method definitions.
If this creates awkward wording, it can always be allowed
in another CL. Also tighten rules for identifying function
references in include comments.

R=briansoman@google.com, caryclark@google.com
TBR=reed@google.com
Bug: skia:6898
Change-Id: I1a0e6b2a76dacfe71d134deb4589fb74e6611a03
Reviewed-on: https://skia-review.googlesource.com/28624
Commit-Queue: Cary Clark <caryclark@skia.org>
Reviewed-by: Cary Clark <caryclark@skia.org>

3 files changed, 68 insertions, 34 deletions

diff --git a/tools/bookmaker/bookmaker.cpp b/tools/bookmaker/bookmaker.cpp
index 3b67663000..7a0e0851fe 100644
--- a/tools/bookmaker/bookmaker.cpp
+++ b/tools/bookmaker/bookmaker.cpp
@@ -48,20 +48,10 @@ more complications I haven't figured out. I don't know when or how to pluralize
 references. This should be "objects' reference counts" probably, but then 
 I lose the link to SkRefCnt.
 
-SkPaint.bmh line 2074
-arcs at front of sentence not capitalized
-
 SkPaint.bmh line 2639
 I'd argue that 'fill path' is OK, in that is it the path that will fill, not the path
 that has already been filled. I see the awkwardness though, and will add it to my bug list.
 
-check for function name in its own description
-
-multiple line #Param / #Return only copies first line?
-
-rework underlinethickness / strikeout thickness
-
-getTextIntercepts lost underline comment
  */
 
 static string normalized_name(string name) {
@@ -1018,6 +1008,19 @@ bool BmhParser::addDefinition(const char* defStart, bool hasEnd, MarkType markTy
                     if (!this->popParentStack(fParent)) { // if not one liner, pop
                         return false;
                     }
+                    if (MarkType::kParam == markType || MarkType::kReturn == markType) {
+                        const char* parmEndCheck = definition->fContentEnd;
+                        while (parmEndCheck < definition->fTerminator) {
+                            if (fMC == parmEndCheck[0]) {
+                                break;
+                            }
+                            if (' ' < parmEndCheck[0]) {
+                                this->reportError<bool>(
+                                        "use full end marker on multiline #Param and #Return");
+                            }
+                            ++parmEndCheck;
+                        }
+                    }
                 } else {
                     fMarkup.emplace_front(markType, defStart, fLineCount, fParent);
                     definition = &fMarkup.front();
diff --git a/tools/bookmaker/bookmaker.h b/tools/bookmaker/bookmaker.h
index 7ad20e6ede..80254505e6 100644
--- a/tools/bookmaker/bookmaker.h
+++ b/tools/bookmaker/bookmaker.h
@@ -1625,19 +1625,21 @@ public:
     void enumMembersOut(const RootDefinition* root, const Definition& child);
     void enumSizeItems(const Definition& child);
     int lookupMethod(const PunctuationState punctuation, const Word word,
-            const int start, const int run, int lastWrite, const char last, 
+            const int start, const int run, int lastWrite, 
             const char* data);
     int lookupReference(const PunctuationState punctuation, const Word word,
             const int start, const int run, int lastWrite, const char last, 
             const char* data);
-    void methodOut(const Definition* method);
+    void methodOut(const Definition* method, const Definition& child);
     bool populate(Definition* def, RootDefinition* root);
     bool populate(BmhParser& bmhParser);
 
     void reset() override {
         INHERITED::resetCommon();
+        fBmhMethod = nullptr;
         fBmhParser = nullptr;
         fEnumDef = nullptr;
+        fMethodDef = nullptr;
         fStructDef = nullptr;
         fAnonymousEnumCount = 1;
         fInStruct = false;
@@ -1654,7 +1656,9 @@ public:
 private:
     BmhParser* fBmhParser;
     Definition* fDeferComment;
+    const Definition* fBmhMethod;
     const Definition* fEnumDef;
+    const Definition* fMethodDef;
     const Definition* fStructDef;
     const char* fContinuation;  // used to construct paren-qualified method name
     int fAnonymousEnumCount;
diff --git a/tools/bookmaker/includeWriter.cpp b/tools/bookmaker/includeWriter.cpp
index d7b5241959..e0cf2714c5 100644
--- a/tools/bookmaker/includeWriter.cpp
+++ b/tools/bookmaker/includeWriter.cpp
@@ -322,7 +322,9 @@ void IncludeWriter::enumSizeItems(const Definition& child) {
 }
 
 // walk children and output complete method doxygen description
-void IncludeWriter::methodOut(const Definition* method) {
+void IncludeWriter::methodOut(const Definition* method, const Definition& child) {
+    fBmhMethod = method;
+    fMethodDef = &child;
     fContinuation = nullptr;
     fDeferComment = nullptr;
     if (0 == fIndent) {
@@ -414,6 +416,8 @@ void IncludeWriter::methodOut(const Definition* method) {
     fIndent -= 4;
     this->lfcr();
     this->writeCommentTrailer();
+    fBmhMethod = nullptr;
+    fMethodDef = nullptr;
 }
 
 void IncludeWriter::structOut(const Definition* root, const Definition& child,
@@ -596,7 +600,7 @@ bool IncludeWriter::populate(Definition* def, RootDefinition* root) {
                     params.skipToEndBracket('(');
                     if (params.fEnd - params.fChar >= childLen &&
                             !strncmp(params.fChar, child.fContentStart, childLen)) {
-                        this->methodOut(clonedMethod);
+                        this->methodOut(clonedMethod, child);
                         break;
                     }
                     ++alternate;
@@ -636,7 +640,7 @@ bool IncludeWriter::populate(Definition* def, RootDefinition* root) {
                     fclose(fOut);  // so we can see what we've written so far
                     return this->reportError<bool>("method not found");
                 }
-                this->methodOut(method);
+                this->methodOut(method, child);
                 continue;
             }
             methodName += "()";
@@ -645,7 +649,7 @@ bool IncludeWriter::populate(Definition* def, RootDefinition* root) {
                 method = method->fParent;
             }
             if (method) {
-                this->methodOut(method);
+                this->methodOut(method, child);
                 continue;
             }
             fLineCount = child.fLineCount;
@@ -680,7 +684,7 @@ bool IncludeWriter::populate(Definition* def, RootDefinition* root) {
                 clonedMethod = method;
                 continue;
             }
-            this->methodOut(method);
+            this->methodOut(method, child);
             continue;
         } 
         if (Definition::Type::kKeyWord == child.fType) {
@@ -903,9 +907,11 @@ static string ConvertRef(const string str, bool first) {
     return substitute;
 }
 
-// FIXME: buggy that this is called with strings containing spaces but resolveRef below is not..
 string IncludeWriter::resolveMethod(const char* start, const char* end, bool first) {
     string methodname(start, end - start);
+    if (string::npos != methodname.find("()")) {
+        return "";
+    }
     string substitute;
     auto rootDefIter = fBmhParser->fMethodMap.find(methodname);
     if (fBmhParser->fMethodMap.end() != rootDefIter) {
@@ -925,6 +931,11 @@ string IncludeWriter::resolveMethod(const char* start, const char* end, bool fir
             substitute = methodname + "()";
         }
     }
+    if (fMethodDef && methodname == fMethodDef->fName) {
+        TextParser report(fBmhMethod);
+        report.reportError("method should not include references to itself");
+        return "";
+    }
     return substitute;
 }
 
@@ -953,6 +964,8 @@ string IncludeWriter::resolveRef(const char* start, const char* end, bool first)
                         }
                     }
                     SkDebugf("unfound: %s\n", undername.c_str());
+                    this->reportError("reference unfound");
+                    return "";
                 }
             }
         }
@@ -1004,25 +1017,30 @@ string IncludeWriter::resolveRef(const char* start, const char* end, bool first)
     }
     return substitute;
 }
+
 int IncludeWriter::lookupMethod(const PunctuationState punctuation, const Word word,
-        const int start, const int run, int lastWrite, const char last, const char* data) {
-    const int end = PunctuationState::kDelimiter == punctuation ||
+        const int lastSpace, const int run, int lastWrite, const char* data) {
+    int wordStart = lastSpace;
+    while (' ' >= data[wordStart]) {
+        ++wordStart;
+    }
+    const int wordEnd = PunctuationState::kDelimiter == punctuation ||
             PunctuationState::kPeriod == punctuation ? run - 1 : run;
-    string temp = this->resolveMethod(&data[start], &data[end], Word::kFirst == word);
+    string temp = this->resolveMethod(&data[wordStart], &data[wordEnd], Word::kFirst == word);
     if (temp.length()) {
-        if (start > lastWrite) {
-            SkASSERT(data[start - 1] >= ' ');
+        if (wordStart > lastWrite) {
+            SkASSERT(data[wordStart - 1] >= ' ');
             if (' ' == data[lastWrite]) {
                 this->writeSpace();
             }
-            this->writeBlockTrim(start - lastWrite, &data[lastWrite]);
-            if (' ' == data[start - 1]) {
+            this->writeBlockTrim(wordStart - lastWrite, &data[lastWrite]);
+            if (' ' == data[wordStart - 1]) {
                 this->writeSpace();
             }
         }
         SkASSERT(temp[temp.length() - 1] > ' ');
         this->writeString(temp.c_str());
-        lastWrite = end;
+        lastWrite = wordEnd;
     }
     return lastWrite;
 }
@@ -1076,8 +1094,10 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
     int lastWrite = 0;
     int lineFeeds = 0;
     int lastPrintable = 0;
+    int lastSpace = -1;
     char c = 0;
     char last;
+    bool embeddedSymbol = false;
     bool hasLower = false;
     bool hasUpper = false;
     bool hasSymbol = false;
@@ -1120,9 +1140,9 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                                 lastWrite, last, data);
                         break;
                     case Word::kMixed:
-                        if (hasUpper && hasLower && !hasSymbol) {
-                            lastWrite = this->lookupMethod(punctuation, word, start, run,
-                                    lastWrite, last, data);
+                        if (hasUpper && hasLower && !hasSymbol && lastSpace > 0) {
+                            lastWrite = this->lookupMethod(punctuation, word, lastSpace, run,
+                                    lastWrite, data);
                         }
                         break;
                     default:
@@ -1132,9 +1152,11 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                         (PunctuationState::kStart == punctuation && ' ' >= last) ? 
                         PunctuationState::kStart : PunctuationState::kSpace;
                 word = Word::kStart;
+                embeddedSymbol = false;
                 hasLower = false;
                 hasUpper = false;
                 hasSymbol = false;
+                lastSpace = run;
                 break;
             case '.':
                 switch (word) {
@@ -1153,10 +1175,9 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                     default:
                         SkASSERT(0);
                 }
-                hasSymbol = true;
+                embeddedSymbol = true;
                 break;
             case ',': case ';': case ':':
-                hasSymbol |= PunctuationState::kDelimiter == punctuation;
                 switch (word) {
                     case Word::kStart:
                         punctuation = PunctuationState::kDelimiter;
@@ -1173,13 +1194,14 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                     default:
                         SkASSERT(0);
                 }
+                embeddedSymbol = true;
                 break;
             case '\'': // possessive apostrophe isn't treated as delimiting punctation
             case '=':
             case '!':  // assumed not to be punctuation, but a programming symbol
             case '&': case '>': case '<': case '{': case '}': case '/': case '*':
                 word = Word::kMixed;
-                hasSymbol = true;
+                embeddedSymbol = true;
                 break;
             case '(':
                 if (' ' == last) {
@@ -1187,11 +1209,11 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                 } else {
                     word = Word::kMixed;
                 }
-                hasSymbol = true;
+                embeddedSymbol = true;
                 break;
             case ')':   // assume word type has already been set
                 punctuation = PunctuationState::kDelimiter;
-                hasSymbol = true;
+                embeddedSymbol = true;
                 break;
             case '_':
                 switch (word) {
@@ -1208,6 +1230,7 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                     default:
                         SkASSERT(0);
                 }
+                hasSymbol |= embeddedSymbol;
                 break;
             case 'A': case 'B': case 'C': case 'D': case 'E':
             case 'F': case 'G': case 'H': case 'I': case 'J':
@@ -1242,6 +1265,7 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                         PunctuationState::kDelimiter == punctuation) {
                     word = Word::kMixed;
                 }
+                hasSymbol |= embeddedSymbol;
                 break;
             case 'a': case 'b': case 'c': case 'd': case 'e':
             case 'f': case 'g': case 'h': case 'i': case 'j':
@@ -1266,6 +1290,7 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
                 }
                 hasLower = true;
                 punctuation = PunctuationState::kStart;
+                hasSymbol |= embeddedSymbol;
                 break;
             default:
                 SkASSERT(0);
@@ -1274,6 +1299,8 @@ IncludeWriter::Wrote IncludeWriter::rewriteBlock(int size, const char* data) {
     }
     if ((word == Word::kCap || word == Word::kFirst || word == Word::kUnderline) && hasLower) {
         lastWrite = this->lookupReference(punctuation, word, start, run, lastWrite, last, data);
+    } else if (word == Word::kMixed && hasUpper && hasLower && !hasSymbol && lastSpace > 0) {
+        lastWrite = this->lookupMethod(punctuation, word, lastSpace, run, lastWrite, data);
     }
     if (run > lastWrite) {
         if (' ' == data[lastWrite]) {