Convert all SkRecordPattern matchers into SkRecord mutators.

- Allow any return type from SkRecord mutators and visitors;
  - update existing calls to mutate and visit;
  - convert match to operator() in SkRecordPattern;
  - tidy up a few inelegant bits of old code in tests.

The net result is that the generated code is much clearer.  All the mutate() calls
inline as you'd hope, and you can now actually follow along with the disassembly.

BUG=skia:2378
R=fmalita@chromium.org, bungeman@google.com, mtklein@google.com

Author: mtklein@chromium.org

Review URL: https://codereview.chromium.org/273643007

git-svn-id: http://skia.googlecode.com/svn/trunk@14631 2bbb7eff-a529-9590-31e7-b0007b416f81

src/record/SkRecord.h

@@ -33,7 +33,7 @@ public:
     ~SkRecord() {
         Destroyer destroyer;
         for (unsigned i = 0; i < this->count(); i++) {
-            this->mutate(i, destroyer);
+            this->mutate<void>(i, destroyer);
         }
     }
 
@@ -42,23 +42,24 @@ public:
 
     // Visit the i-th canvas command with a functor matching this interface:
     //   template <typename T>
-    //   void operator()(const T& record) { ... }
+    //   R operator()(const T& record) { ... }
     // This operator() must be defined for at least all SkRecords::*.
-    template <typename F>
-    void visit(unsigned i, F& f) const {
+    template <typename R, typename F>
+    R visit(unsigned i, F& f) const {
         SkASSERT(i < this->count());
-        fRecords[i].visit(fTypes[i], f);
+        return fRecords[i].visit<R>(fTypes[i], f);
     }
 
     // Mutate the i-th canvas command with a functor matching this interface:
     //   template <typename T>
-    //   void operator()(T* record) { ... }
+    //   R operator()(T* record) { ... }
     // This operator() must be defined for at least all SkRecords::*.
-    template <typename F>
-    void mutate(unsigned i, F& f) {
+    template <typename R, typename F>
+    R mutate(unsigned i, F& f) {
         SkASSERT(i < this->count());
-        fRecords[i].mutate(fTypes[i], f);
+        return fRecords[i].mutate<R>(fTypes[i], f);
     }
+    // TODO: It'd be nice to infer R from F for visit and mutate if we ever get std::result_of.
 
     // Allocate contiguous space for count Ts, to be freed when the SkRecord is destroyed.
     // Here T can be any class, not just those from SkRecords.  Throws on failure.
@@ -89,7 +90,7 @@ public:
         SkASSERT(i < this->count());
 
         Destroyer destroyer;
-        this->mutate(i, destroyer);
+        this->mutate<void>(i, destroyer);
 
         fTypes[i] = T::kType;
         return fRecords[i].set(this->allocCommand<T>());
@@ -191,20 +192,24 @@ private:
 
         // Visit this record with functor F (see public API above) assuming the record we're
         // pointing to has this type.
-        template <typename F>
-        void visit(Type8 type, F& f) const {
+        template <typename R, typename F>
+        R visit(Type8 type, F& f) const {
         #define CASE(T) case SkRecords::T##_Type: return f(*this->ptr<SkRecords::T>());
             switch(type) { SK_RECORD_TYPES(CASE) }
         #undef CASE
+            SkDEBUGFAIL("Unreachable");
+            return R();
         }
 
         // Mutate this record with functor F (see public API above) assuming the record we're
         // pointing to has this type.
-        template <typename F>
-        void mutate(Type8 type, F& f) {
+        template <typename R, typename F>
+        R mutate(Type8 type, F& f) {
         #define CASE(T) case SkRecords::T##_Type: return f(this->ptr<SkRecords::T>());
             switch(type) { SK_RECORD_TYPES(CASE) }
         #undef CASE
+            SkDEBUGFAIL("Unreachable");
+            return R();
         }
 
     private:

src/record/SkRecordDraw.cpp

@@ -94,6 +94,6 @@ template <> void Draw::draw(const SkRecords::BoundedDrawPosTextH& r) { this->dra
 
 void SkRecordDraw(const SkRecord& record, SkCanvas* canvas) {
     for (Draw draw(canvas); draw.index() < record.count(); draw.next()) {
-        record.visit(draw.index(), draw);
+        record.visit<void>(draw.index(), draw);
     }
 }

src/record/SkRecordOpts.cpp

@@ -264,7 +264,7 @@ public:
 
     void apply(SkRecord* record) {
         for (fRecord = record, fIndex = 0; fIndex < record->count(); fIndex++) {
-            fRecord->mutate(fIndex, *this);
+            fRecord->mutate<void>(fIndex, *this);
         }
     }
 

src/record/SkRecordPattern.h

@@ -17,13 +17,13 @@ public:
     typedef T type;
     type* get() { return fPtr; }
 
-    bool match(T* ptr) {
+    bool operator()(T* ptr) {
         fPtr = ptr;
         return true;
     }
 
     template <typename U>
-    bool match(U*) {
+    bool operator()(U*) {
         fPtr = NULL;
         return false;
     }
@@ -42,19 +42,19 @@ public:
     type* get() { return fPaint; }
 
     template <typename T>
-    SK_WHEN(HasMember_paint<T>, bool) match(T* draw) {
+    SK_WHEN(HasMember_paint<T>, bool) operator()(T* draw) {
         fPaint = AsPtr(draw->paint);
         return true;
     }
 
     template <typename T>
-    SK_WHEN(!HasMember_paint<T>, bool) match(T*) {
+    SK_WHEN(!HasMember_paint<T>, bool) operator()(T*) {
         fPaint = NULL;
         return false;
     }
 
     // SaveLayer has an SkPaint named paint, but it's not a draw.
-    bool match(SaveLayer*) {
+    bool operator()(SaveLayer*) {
         fPaint = NULL;
         return false;
     }
@@ -71,14 +71,14 @@ private:
 template <typename Matcher>
 struct Not {
     template <typename T>
-    bool match(T* ptr) { return !Matcher().match(ptr); }
+    bool operator()(T* ptr) { return !Matcher()(ptr); }
 };
 
 // Matches if either of A or B does.  Stores nothing.
 template <typename A, typename B>
 struct Or {
     template <typename T>
-    bool match(T* ptr) { return A().match(ptr) || B().match(ptr); }
+    bool operator()(T* ptr) { return A()(ptr) || B()(ptr); }
 };
 
 // Matches if any of A, B or C does.  Stores nothing.
@@ -96,7 +96,7 @@ public:
     void reset() {}
 
     template <typename T>
-    bool match(T* ptr) { return Matcher().match(ptr); }
+    bool operator()(T* ptr) { return Matcher()(ptr); }
 };
 
 // This version stores a list of matches.  It's enabled if Matcher stores something.
@@ -109,9 +109,9 @@ public:
     void reset() { fMatches.rewind(); }
 
     template <typename T>
-    bool match(T* ptr) {
+    bool operator()(T* ptr) {
         Matcher matcher;
-        if (matcher.match(ptr)) {
+        if (matcher(ptr)) {
             fMatches.push(matcher.get());
             return true;
         }
@@ -161,16 +161,11 @@ public:
     template <typename T> T* third()  { return fTail.fTail.fHead.get(); }
 
 private:
-    template <typename T>
-    void operator()(T* r) { fHeadMatched = fHead.match(r); }
-
     // If head isn't a Star, try to match at i once.
     template <typename T>
     unsigned matchHead(T*, SkRecord* record, unsigned i) {
         if (i < record->count()) {
-            fHeadMatched = false;
-            record->mutate(i, *this);
-            if (fHeadMatched) {
+            if (record->mutate<bool>(i, fHead)) {
                 return i+1;
             }
         }
@@ -182,9 +177,7 @@ private:
     unsigned matchHead(Star<T>*, SkRecord* record, unsigned i) {
         fHead.reset();
         while (i < record->count()) {
-            fHeadMatched = false;
-            record->mutate(i, *this);
-            if (!fHeadMatched) {
+            if (!record->mutate<bool>(i, fHead)) {
                 return i;
             }
             i++;
@@ -194,9 +187,6 @@ private:
 
     Matcher fHead;
     Pattern fTail;
-    bool fHeadMatched;
-
-    friend class ::SkRecord;  // So operator() can otherwise stay private.
 
     // All Cons are friends with each other.  This lets first, second, and third work.
     template <typename, typename> friend class Cons;

tests/RecordOptsTest.cpp

@@ -19,26 +19,25 @@ static const int W = 1920, H = 1080;
 // If the command we're reading is a U, set ptr to it, otherwise set it to NULL.
 template <typename U>
 struct ReadAs {
-    explicit ReadAs(const U** ptr) : ptr(ptr), type(SkRecords::Type(~0)) {}
+    ReadAs() : ptr(NULL), type(SkRecords::Type(~0)) {}
 
-    const U** ptr;
+    const U* ptr;
     SkRecords::Type type;
 
-    void operator()(const U& r) { *ptr = &r; type = U::kType; }
+    void operator()(const U& r) { ptr = &r; type = U::kType; }
 
     template <typename T>
-    void operator()(const T&) { *ptr = NULL; type = U::kType; }
+    void operator()(const T&) { type = U::kType; }
 };
 
 // Assert that the ith command in record is of type T, and return it.
 template <typename T>
 static const T* assert_type(skiatest::Reporter* r, const SkRecord& record, unsigned index) {
-    const T* ptr = NULL;
-    ReadAs<T> reader(&ptr);
-    record.visit(index, reader);
+    ReadAs<T> reader;
+    record.visit<void>(index, reader);
     REPORTER_ASSERT(r, T::kType == reader.type);
-    REPORTER_ASSERT(r, ptr != NULL);
-    return ptr;
+    REPORTER_ASSERT(r, NULL != reader.ptr);
+    return reader.ptr;
 }
 
 DEF_TEST(RecordOpts_Culling, r) {

tests/RecordTest.cpp

@@ -17,35 +17,36 @@ public:
 
     template <typename T> void operator()(const T&) { }
 
+    void operator()(const SkRecords::DrawRect& draw) {
+        fArea += (int)(draw.rect.width() * draw.rect.height());
+    }
+
     int area() const { return fArea; }
 
     void apply(const SkRecord& record) {
         for (unsigned i = 0; i < record.count(); i++) {
-            record.visit(i, *this);
+            record.visit<void>(i, *this);
         }
     }
 
 private:
     int fArea;
 };
-template <> void AreaSummer::operator()(const SkRecords::DrawRect& record) {
-    fArea += (int) (record.rect.width() * record.rect.height());
-}
 
 // Scales out the bottom-right corner of any DrawRect command it sees by 2x.
 struct Stretch {
     template <typename T> void operator()(T*) {}
+    void operator()(SkRecords::DrawRect* draw) {
+        draw->rect.fRight *= 2;
+        draw->rect.fBottom *= 2;
+    }
 
     void apply(SkRecord* record) {
         for (unsigned i = 0; i < record->count(); i++) {
-            record->mutate(i, *this);
+            record->mutate<void>(i, *this);
         }
     }
 };
-template <> void Stretch::operator()(SkRecords::DrawRect* record) {
-    record->rect.fRight *= 2;
-    record->rect.fBottom *= 2;
-}
 
 // Basic tests for the low-level SkRecord code.
 DEF_TEST(Record, r) {

tests/RecorderTest.cpp

@@ -30,7 +30,7 @@ public:
 
     void apply(const SkRecord& record) {
         for (unsigned i = 0; i < record.count(); i++) {
-            record.visit(i, *this);
+            record.visit<void>(i, *this);
         }
     }
 

tools/dump_record.cpp

@@ -84,7 +84,7 @@ static void dump(const char* name, const SkRecord& record) {
     printf("%s %s\n", FLAGS_optimize ? "optimized" : "not-optimized", name);
     for (unsigned i = 0; i < record.count(); i++) {
         printf("%*d ", digits, i);
-        record.visit(i, dumper);
+        record.visit<void>(i, dumper);
     }
 }