Refactor gmmain to separate out config parsing

gm/gmmain.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 /*
  * Code for the "gm" (Golden Master) rendering comparison tool.
  *
@@ skipping 1749 matching lines @@
             SkString subdir;
             subdir.appendf("%s%c%s", root, SkPATH_SEPARATOR, config.fName);
             if (!sk_mkdir(subdir.c_str())) {
                 return false;
             }
         }
     }
     return true;
 }
 
-int tool_main(int argc, char** argv);
-int tool_main(int argc, char** argv) {
-
-#if SK_ENABLE_INST_COUNT
-    gPrintInstCount = true;
-#endif
-
-    SkGraphics::Init();
-    // we don't need to see this during a run
-    gSkSuppressFontCachePurgeSpew = true;
-
-    setSystemPreferences();
-    GMMain gmmain;
-
-    SkTDArray<size_t> configs;
+bool parse_flags_configs(SkTDArray<size_t>* outConfigs,

[edisonn, 2013/08/02 12:32:41]

static

[ducky, 2013/08/02 18:14:38]

Fixed as of latest patchset last night, after I realized the Mac build broke. I
guess the review request doesn't automatically track updated patchsets, so I'll
re-send review requests as this stuff gets updated.

+                         GrContextFactory* grFactory) {
     SkTDArray<size_t> excludeConfigs;
-    bool userConfig = false;
-
-    SkString usage;
-    usage.printf("Run the golden master tests.\n");
-    SkCommandLineFlags::SetUsage(usage.c_str());
-    SkCommandLineFlags::Parse(argc, argv);
-
-    gmmain.fUseFileHierarchy = FLAGS_hierarchy;
-    gmmain.fWriteChecksumBasedFilenames = FLAGS_writeChecksumBasedFilenames;
-    if (FLAGS_mismatchPath.count() == 1) {
-        gmmain.fMismatchPath = FLAGS_mismatchPath[0];
-    }
-    if (FLAGS_missingExpectationsPath.count() == 1) {
-        gmmain.fMissingExpectationsPath = FLAGS_missingExpectationsPath[0];
-    }
 
     for (int i = 0; i < FLAGS_config.count(); i++) {
         const char* config = FLAGS_config[i];
-        userConfig = true;
         bool exclude = false;
         if (*config == kExcludeConfigChar) {
             exclude = true;
             config += 1;
         }
         int index = findConfig(config);
         if (index >= 0) {
             if (exclude) {
                 *excludeConfigs.append() = index;
             } else {
-                appendUnique<size_t>(&configs, index);
+                appendUnique<size_t>(outConfigs, index);
             }
         } else if (0 == strcmp(kDefaultsConfigStr, config)) {
+            if (exclude) {
+                gm_fprintf(stderr, "%c%s is not allowed.\n",
+                           kExcludeConfigChar, kDefaultsConfigStr);
+                return false;
+            }
             for (size_t c = 0; c < SK_ARRAY_COUNT(gRec); ++c) {
                 if (gRec[c].fRunByDefault) {
-                    if (exclude) {
-                        gm_fprintf(stderr, "%c%s is not allowed.\n",
-                                   kExcludeConfigChar, kDefaultsConfigStr);
-                        return -1;
-                    } else {
-                        appendUnique<size_t>(&configs, c);
-                    }
+                    appendUnique<size_t>(outConfigs, c);
                 }
             }
         } else {
             gm_fprintf(stderr, "unrecognized config %s\n", config);
-            return -1;
+            return false;
         }
     }
 
     for (int i = 0; i < FLAGS_excludeConfig.count(); i++) {
         int index = findConfig(FLAGS_excludeConfig[i]);
         if (index >= 0) {
             *excludeConfigs.append() = index;
         } else {
             gm_fprintf(stderr, "unrecognized excludeConfig %s\n", FLAGS_excludeConfig[i]);
-            return -1;
-        }
-    }
-
-    int moduloRemainder = -1;
-    int moduloDivisor = -1;
-
-    if (FLAGS_modulo.count() == 2) {
-        moduloRemainder = atoi(FLAGS_modulo[0]);
-        moduloDivisor = atoi(FLAGS_modulo[1]);
-        if (moduloRemainder < 0 || moduloDivisor <= 0 || moduloRemainder >= moduloDivisor) {
-            gm_fprintf(stderr, "invalid modulo values.");
-            return -1;
-        }
-    }
-
+            return false;
+        }
+    }
+
+    if (outConfigs->count() == 0) {
+        // if no config is specified by user, add the defaults
+        for (size_t i = 0; i < SK_ARRAY_COUNT(gRec); ++i) {
+            if (gRec[i].fRunByDefault) {
+                *outConfigs->append() = i;
+            }
+        }
+    }
+    // now remove any explicitly excluded configs
+    for (int i = 0; i < excludeConfigs.count(); ++i) {
+        int index = outConfigs->find(excludeConfigs[i]);
+        if (index >= 0) {
+            outConfigs->remove(index);
+            // now assert that there was only one copy in configs[]
+            SkASSERT(outConfigs->find(excludeConfigs[i]) < 0);
+        }
+    }
+
+    if (grFactory != NULL) {
+        for (int i = 0; i < outConfigs->count(); ++i) {
+            size_t index = (*outConfigs)[i];
+            if (kGPU_Backend == gRec[index].fBackend) {
+                GrContext* ctx = grFactory->get(gRec[index].fGLContextType);
+                if (NULL == ctx) {
+                    gm_fprintf(stderr, "GrContext could not be created for config %s."
+                               " Config will be skipped.\n", gRec[index].fName);
+                    outConfigs->remove(i);
+                    --i;
+                    continue;
+                }
+                if (gRec[index].fSampleCnt > ctx->getMaxSampleCount()) {
+                    gm_fprintf(stderr, "Sample count (%d) of config %s is not supported."
+                               " Config will be skipped.\n",
+                               gRec[index].fSampleCnt, gRec[index].fName);
+                    outConfigs->remove(i);
+                    --i;
+                }
+            }
+        }
+    }
+
+    if (outConfigs->isEmpty()) {
+        gm_fprintf(stderr, "No configs to run.");
+        return false;
+    }
+
+    // now show the user the set of configs that will be run.
+    SkString configStr("These configs will be run: ");
+    // show the user the config that will run.
+    for (int i = 0; i < outConfigs->count(); ++i) {
+        configStr.appendf("%s%s", gRec[(*outConfigs)[i]].fName,
+                                  (i == outConfigs->count() - 1) ? "\n" : " ");

[edisonn, 2013/08/02 12:32:41]

make it (i < outConfigs->count()) ? " " : "");

and add an append of \n after the for. it is more clear that way.

[ducky, 2013/08/02 18:14:38]

Done. It now just adds a space all the time, since trailing spaces are invisible
anyways.

+    }
+    gm_fprintf(stdout, "%s", configStr.c_str());
+
+    return true;
+}
+
+bool parse_flags_ignore_error_types(ErrorCombination* outErrorTypes) {

[edisonn, 2013/08/02 12:32:41]

static

[ducky, 2013/08/02 18:14:38]

Done.

     if (FLAGS_ignoreErrorTypes.count() > 0) {
-        gmmain.fIgnorableErrorTypes = ErrorCombination();
+        *outErrorTypes = ErrorCombination();
         for (int i = 0; i < FLAGS_ignoreErrorTypes.count(); i++) {
             ErrorType type;
             const char *name = FLAGS_ignoreErrorTypes[i];
             if (!getErrorTypeByName(name, &type)) {
                 gm_fprintf(stderr, "cannot find ErrorType with name '%s'\n", name);
-                return -1;
+                return false;
             } else {
-                gmmain.fIgnorableErrorTypes.add(type);
-            }
-        }
-    }
-
-#if SK_SUPPORT_GPU
+                outErrorTypes->add(type);
+            }
+        }
+    }
+    return true;
+}
+
+bool parse_flags_modulo(int* moduloRemainder, int* moduloDivisor) {

[edisonn, 2013/08/02 12:32:41]

make it static, otherwise you might get a warning as error on a few bots!

[ducky, 2013/08/02 18:14:38]

Done.

+    if (FLAGS_modulo.count() == 2) {
+        *moduloRemainder = atoi(FLAGS_modulo[0]);
+        *moduloDivisor = atoi(FLAGS_modulo[1]);
+        if (*moduloRemainder < 0 || *moduloDivisor <= 0 ||
+                *moduloRemainder >= *moduloDivisor) {
+            gm_fprintf(stderr, "invalid modulo values.");
+            return false;
+        }
+    }
+    return true;
+}
+
+bool parse_flags_gpu_cache(int* sizeBytes, int* sizeCount) {

[edisonn, 2013/08/02 12:32:41]

static

[ducky, 2013/08/02 18:14:38]

Done.

     if (FLAGS_gpuCacheSize.count() > 0) {
         if (FLAGS_gpuCacheSize.count() != 2) {
             gm_fprintf(stderr, "--gpuCacheSize requires two arguments\n");
-            return -1;
-        }
-        gGpuCacheSizeBytes = atoi(FLAGS_gpuCacheSize[0]);
-        gGpuCacheSizeCount = atoi(FLAGS_gpuCacheSize[1]);
+            return false;
+        }
+        *sizeBytes = atoi(FLAGS_gpuCacheSize[0]);
+        *sizeCount = atoi(FLAGS_gpuCacheSize[1]);
     } else {
-        gGpuCacheSizeBytes = DEFAULT_CACHE_VALUE;
-        gGpuCacheSizeCount = DEFAULT_CACHE_VALUE;
-    }
-#endif
-
-    SkTDArray<SkScalar> tileGridReplayScales;
-    *tileGridReplayScales.append() = SK_Scalar1; // By default only test at scale 1.0
+        *sizeBytes = DEFAULT_CACHE_VALUE;
+        *sizeCount = DEFAULT_CACHE_VALUE;
+    }
+    return true;
+}
+
+bool parse_tile_grid_replay_scales(SkTDArray<SkScalar>* outScales) {

[edisonn, 2013/08/02 12:32:41]

static

[ducky, 2013/08/02 18:14:38]

Done.

+    *outScales->append() = SK_Scalar1; // By default only test at scale 1.0
     if (FLAGS_tileGridReplayScales.count() > 0) {
-        tileGridReplayScales.reset();
+        outScales->reset();
         for (int i = 0; i < FLAGS_tileGridReplayScales.count(); i++) {
             double val = atof(FLAGS_tileGridReplayScales[i]);
             if (0 < val) {
-                *tileGridReplayScales.append() = SkDoubleToScalar(val);
-            }
-        }
-        if (0 == tileGridReplayScales.count()) {
+                *outScales->append() = SkDoubleToScalar(val);
+            }
+        }
+        if (0 == outScales->count()) {
             // Should have at least one scale
             gm_fprintf(stderr, "--tileGridReplayScales requires at least one scale.\n");
-            return -1;
-        }
-    }
-
-    if (!userConfig) {
-        // if no config is specified by user, add the defaults
-        for (size_t i = 0; i < SK_ARRAY_COUNT(gRec); ++i) {
-            if (gRec[i].fRunByDefault) {
-                *configs.append() = i;
-            }
-        }
-    }
-    // now remove any explicitly excluded configs
-    for (int i = 0; i < excludeConfigs.count(); ++i) {
-        int index = configs.find(excludeConfigs[i]);
-        if (index >= 0) {
-            configs.remove(index);
-            // now assert that there was only one copy in configs[]
-            SkASSERT(configs.find(excludeConfigs[i]) < 0);
-        }
-    }
-
-#if SK_SUPPORT_GPU
-    GrContextFactory* grFactory = new GrContextFactory;
-    for (int i = 0; i < configs.count(); ++i) {
-        size_t index = configs[i];
-        if (kGPU_Backend == gRec[index].fBackend) {
-            GrContext* ctx = grFactory->get(gRec[index].fGLContextType);
-            if (NULL == ctx) {
-                gm_fprintf(stderr, "GrContext could not be created for config %s."
-                           " Config will be skipped.\n", gRec[index].fName);
-                configs.remove(i);
-                --i;
-                continue;
-            }
-            if (gRec[index].fSampleCnt > ctx->getMaxSampleCount()) {
-                gm_fprintf(stderr, "Sample count (%d) of config %s is not supported."
-                           " Config will be skipped.\n", gRec[index].fSampleCnt, gRec[index].fName);
-                configs.remove(i);
-                --i;
-            }
-        }
-    }
-#else
-    GrContextFactory* grFactory = NULL;
-#endif
-
-    if (configs.isEmpty()) {
-        gm_fprintf(stderr, "No configs to run.");
-        return -1;
-    }
-
-    // now show the user the set of configs that will be run.
-    SkString configStr("These configs will be run: ");
-    // show the user the config that will run.
-    for (int i = 0; i < configs.count(); ++i) {
-        configStr.appendf("%s%s", gRec[configs[i]].fName, (i == configs.count() - 1) ? "\n" : " ");
-    }
-    gm_fprintf(stdout, "%s", configStr.c_str());
-
-    if (FLAGS_resourcePath.count() == 1) {
-        GM::SetResourcePath(FLAGS_resourcePath[0]);
-    }
-
+            return false;
+        }
+    }
+    return true;
+}
+
+bool parse_read_path(SkAutoTUnref<ExpectationsSource>* source) {

[edisonn, 2013/08/02 12:32:41]

static

[ducky, 2013/08/02 18:14:38]

Done.

     if (FLAGS_readPath.count() == 1) {
         const char* readPath = FLAGS_readPath[0];
         if (!sk_exists(readPath)) {
             gm_fprintf(stderr, "readPath %s does not exist!\n", readPath);
-            return -1;
+            return false;
         }
         if (sk_isdir(readPath)) {
             if (FLAGS_verbose) {
                 gm_fprintf(stdout, "reading from %s\n", readPath);
             }
-            gmmain.fExpectationsSource.reset(SkNEW_ARGS(
+            source->reset(SkNEW_ARGS(
                 IndividualImageExpectationsSource, (readPath)));
         } else {
             if (FLAGS_verbose) {
                 gm_fprintf(stdout, "reading expectations from JSON summary file %s\n", readPath);
             }
-            gmmain.fExpectationsSource.reset(SkNEW_ARGS(
-                JsonExpectationsSource, (readPath)));
-        }
-    }
+            source->reset(SkNEW_ARGS(JsonExpectationsSource, (readPath)));
+        }
+    }
+    return true;
+}
+
+bool parse_match_strs(SkTDArray<const char*>* matchStrs) {

[edisonn, 2013/08/02 12:32:41]

static

[ducky, 2013/08/02 18:14:38]

Done.

+    for (int i = 0; i < FLAGS_match.count(); ++i) {
+        matchStrs->push(FLAGS_match[i]);
+    }
+    return true;
+}
+
+int tool_main(int argc, char** argv);
+int tool_main(int argc, char** argv) {
+
+#if SK_ENABLE_INST_COUNT
+    gPrintInstCount = true;
+#endif
+
+    SkGraphics::Init();
+    // we don't need to see this during a run
+    gSkSuppressFontCachePurgeSpew = true;
+
+    setSystemPreferences();
+    GMMain gmmain;
+
+    SkString usage;
+    usage.printf("Run the golden master tests.\n");
+    SkCommandLineFlags::SetUsage(usage.c_str());
+    SkCommandLineFlags::Parse(argc, argv);
+
+    gmmain.fUseFileHierarchy = FLAGS_hierarchy;
+    gmmain.fWriteChecksumBasedFilenames = FLAGS_writeChecksumBasedFilenames;
+    if (FLAGS_mismatchPath.count() == 1) {
+        gmmain.fMismatchPath = FLAGS_mismatchPath[0];
+    }
+    if (FLAGS_missingExpectationsPath.count() == 1) {
+        gmmain.fMissingExpectationsPath = FLAGS_missingExpectationsPath[0];
+    }
+
+    SkTDArray<size_t> configs;
+    int moduloRemainder = -1, moduloDivisor = -1;
+    SkTDArray<SkScalar> tileGridReplayScales;
+#if SK_SUPPORT_GPU
+    GrContextFactory* grFactory = new GrContextFactory;
+#else
+    GrContextFactory* grFactory = NULL;
+#endif
+    SkTDArray<const char*> matchStrs;
+
+    if (!parse_flags_configs(&configs, grFactory) ||
+        !parse_flags_ignore_error_types(&(gmmain.fIgnorableErrorTypes)) ||
+        !parse_flags_modulo(&moduloRemainder, &moduloDivisor) ||
+#if SK_SUPPORT_GPU
+        !parse_flags_gpu_cache(&gGpuCacheSizeBytes, &gGpuCacheSizeCount) ||
+#endif
+        !parse_tile_grid_replay_scales(&tileGridReplayScales) ||
+        !parse_read_path(&gmmain.fExpectationsSource) ||
+        !parse_match_strs(&matchStrs)) {
+        return -1;
+    }
+
+    if (FLAGS_resourcePath.count() == 1) {
+        GM::SetResourcePath(FLAGS_resourcePath[0]);
+    }
+
     if (FLAGS_verbose) {
         if (FLAGS_writePath.count() == 1) {
             gm_fprintf(stdout, "writing to %s\n", FLAGS_writePath[0]);
         }
         if (NULL != gmmain.fMismatchPath) {
             gm_fprintf(stdout, "writing mismatches to %s\n", gmmain.fMismatchPath);
         }
         if (NULL != gmmain.fMissingExpectationsPath) {
             gm_fprintf(stdout, "writing images without expectations to %s\n",
                        gmmain.fMissingExpectationsPath);
         }
         if (FLAGS_writePicturePath.count() == 1) {
             gm_fprintf(stdout, "writing pictures to %s\n", FLAGS_writePicturePath[0]);
         }
         if (FLAGS_resourcePath.count() == 1) {
             gm_fprintf(stdout, "reading resources from %s\n", FLAGS_resourcePath[0]);
         }
     }
 
-    if (moduloDivisor <= 0) {
-        moduloRemainder = -1;
-    }
-    if (moduloRemainder < 0 || moduloRemainder >= moduloDivisor) {
-        moduloRemainder = -1;
-    }
-
     int gmsRun = 0;
     int gmIndex = -1;
     SkString moduloStr;
 
     // If we will be writing out files, prepare subdirectories.
     if (FLAGS_writePath.count() == 1) {
         if (!prepare_subdirectories(FLAGS_writePath[0], gmmain.fUseFileHierarchy, configs)) {
             return -1;
         }
     }
     if (NULL != gmmain.fMismatchPath) {
         if (!prepare_subdirectories(gmmain.fMismatchPath, gmmain.fUseFileHierarchy, configs)) {
             return -1;
         }
     }
     if (NULL != gmmain.fMissingExpectationsPath) {
         if (!prepare_subdirectories(gmmain.fMissingExpectationsPath, gmmain.fUseFileHierarchy,
                                     configs)) {
             return -1;
         }
     }
 
     if (FLAGS_pdfJpegQuality < -1 || FLAGS_pdfJpegQuality > 100) {

[ducky, 2013/08/02 03:59:04]

Is this supposed to do anything, like exit the program? Or is it just a warning?

[edisonn, 2013/08/02 12:32:41]

please add return -1 here. good find.

[ducky, 2013/08/02 18:14:38]

Done.

         gm_fprintf(stderr, "%s\n", "pdfJpegQuality must be in [-1 .. 100] range.");
     }
 
     Iter iter;
     GM* gm;
     while ((gm = iter.next()) != NULL) {
         SkAutoTDelete<GM> adgm(gm);
         ++gmIndex;
         if (moduloRemainder >= 0) {
             if ((gmIndex % moduloDivisor) != moduloRemainder) {
                 continue;
             }
             moduloStr.printf("[%d.%d] ", gmIndex, moduloDivisor);
         }
 
         const char* shortName = gm->shortName();
 
-        SkTDArray<const char*> matchStrs;
-        for (int i = 0; i < FLAGS_match.count(); ++i) {
-            matchStrs.push(FLAGS_match[i]);
-        }
         if (SkCommandLineFlags::ShouldSkip(matchStrs, shortName)) {
             continue;
         }
 
         gmsRun++;
         SkISize size = gm->getISize();
         gm_fprintf(stdout, "%sdrawing... %s [%d %d]\n", moduloStr.c_str(), shortName,
                    size.width(), size.height());
 
         run_multiple_configs(gmmain, gm, configs, grFactory);
@@ skipping 73 matching lines @@
     if (FLAGS_forceBWtext) {
         canvas->setDrawFilter(SkNEW(BWTextDrawFilter))->unref();
     }
 }
 
 #if !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_NACL)
 int main(int argc, char * const argv[]) {
     return tool_main(argc, (char**) argv);
 }
 #endif