Merge "Reduce warning verbosity in aapt"

4 files changed, 98 insertions, 92 deletions

diff --git a/tools/aapt/ResourceTable.cpp b/tools/aapt/ResourceTable.cpp
index aff0088..652998e 100644
--- a/tools/aapt/ResourceTable.cpp
+++ b/tools/aapt/ResourceTable.cpp
@@ -1317,9 +1317,9 @@ status_t compileResourceFile(Bundle* bundle,
                         curIsFormatted = false;
                         // Untranslatable strings must only exist in the default [empty] locale
                         if (locale.size() > 0) {
-                            fprintf(stderr, "aapt: warning: string '%s' in %s marked untranslatable but exists"
-                                    " in locale '%s'\n", String8(name).string(),
-                                    bundle->getResourceSourceDirs()[0],
+                            SourcePos(in->getPrintableSource(), block.getLineNumber()).warning(
+                                    "string '%s' marked untranslatable but exists in locale '%s'\n",
+                                    String8(name).string(),
                                     locale.string());
                             // hasErrors = localHasErrors = true;
                         } else {
@@ -1330,7 +1330,10 @@ status_t compileResourceFile(Bundle* bundle,
                             // having no default translation.
                         }
                     } else {
-                        outTable->addLocalization(name, locale);
+                        outTable->addLocalization(
+                                name,
+                                locale,
+                                SourcePos(in->getPrintableSource(), block.getLineNumber()));
                     }
 
                     if (formatted == false16) {
@@ -2570,9 +2573,9 @@ status_t ResourceTable::addSymbols(const sp<AaptSymbols>& outSymbols) {
 
 
 void
-ResourceTable::addLocalization(const String16& name, const String8& locale)
+ResourceTable::addLocalization(const String16& name, const String8& locale, const SourcePos& src)
 {
-    mLocalizations[name].insert(locale);
+    mLocalizations[name][locale] = src;
 }
 
 
@@ -2592,21 +2595,22 @@ ResourceTable::validateLocalizations(void)
     const String8 defaultLocale;
 
     // For all strings...
-    for (map<String16, set<String8> >::iterator nameIter = mLocalizations.begin();
+    for (map<String16, map<String8, SourcePos> >::iterator nameIter = mLocalizations.begin();
          nameIter != mLocalizations.end();
          nameIter++) {
-        const set<String8>& configSet = nameIter->second;   // naming convenience
+        const map<String8, SourcePos>& configSrcMap = nameIter->second;
 
         // Look for strings with no default localization
-        if (configSet.count(defaultLocale) == 0) {
-            fprintf(stdout, "aapt: warning: string '%s' has no default translation in %s; found:",
-                    String8(nameIter->first).string(), mBundle->getResourceSourceDirs()[0]);
-            for (set<String8>::const_iterator locales = configSet.begin();
-                 locales != configSet.end();
-                 locales++) {
-                fprintf(stdout, " %s", (*locales).string());
-            }
-            fprintf(stdout, "\n");
+        if (configSrcMap.count(defaultLocale) == 0) {
+            SourcePos().warning("string '%s' has no default translation.",
+                    String8(nameIter->first).string());
+            if (mBundle->getVerbose()) {
+                for (map<String8, SourcePos>::const_iterator locales = configSrcMap.begin();
+                    locales != configSrcMap.end();
+                    locales++) {
+                    locales->second.printf("locale %s found", locales->first.string());
+                }
+            }
             // !!! TODO: throw an error here in some circumstances
         }
 
@@ -2616,6 +2620,8 @@ ResourceTable::validateLocalizations(void)
             const char* start = allConfigs;
             const char* comma;
             
+            set<String8> missingConfigs;
+            AaptLocaleValue locale;
             do {
                 String8 config;
                 comma = strchr(start, ',');
@@ -2626,27 +2632,38 @@ ResourceTable::validateLocalizations(void)
                     config.setTo(start);
                 }
 
+                if (!locale.initFromFilterString(config)) {
+                    continue;
+                }
+
                 // don't bother with the pseudolocale "zz_ZZ"
                 if (config != "zz_ZZ") {
-                    if (configSet.find(config) == configSet.end()) {
+                    if (configSrcMap.find(config) == configSrcMap.end()) {
                         // okay, no specific localization found.  it's possible that we are
                         // requiring a specific regional localization [e.g. de_DE] but there is an
                         // available string in the generic language localization [e.g. de];
                         // consider that string to have fulfilled the localization requirement.
                         String8 region(config.string(), 2);
-                        if (configSet.find(region) == configSet.end()) {
-                            if (configSet.count(defaultLocale) == 0) {
-                                fprintf(stdout, "aapt: warning: "
-                                        "**** string '%s' has no default or required localization "
-                                        "for '%s' in %s\n",
-                                        String8(nameIter->first).string(),
-                                        config.string(),
-                                        mBundle->getResourceSourceDirs()[0]);
-                            }
+                        if (configSrcMap.find(region) == configSrcMap.end() &&
+                                configSrcMap.count(defaultLocale) == 0) {
+                            missingConfigs.insert(config);
                         }
                     }
                 }
-           } while (comma != NULL);
+            } while (comma != NULL);
+
+            if (!missingConfigs.empty()) {
+                String8 configStr;
+                for (set<String8>::iterator iter = missingConfigs.begin();
+                     iter != missingConfigs.end();
+                     iter++) {
+                    configStr.appendFormat(" %s", iter->string());
+                }
+                SourcePos().warning("string '%s' is missing %u required localizations:%s",
+                        String8(nameIter->first).string(),
+                        (unsigned int)missingConfigs.size(),
+                        configStr.string());
+            }
         }
     }
 
diff --git a/tools/aapt/ResourceTable.h b/tools/aapt/ResourceTable.h
index a3e0666..75005cd 100644
--- a/tools/aapt/ResourceTable.h
+++ b/tools/aapt/ResourceTable.h
@@ -220,7 +220,7 @@ public:
 
     status_t assignResourceIds();
     status_t addSymbols(const sp<AaptSymbols>& outSymbols = NULL);
-    void addLocalization(const String16& name, const String8& locale);
+    void addLocalization(const String16& name, const String8& locale, const SourcePos& src);
     status_t validateLocalizations(void);
 
     status_t flatten(Bundle*, const sp<AaptFile>& dest);
@@ -551,7 +551,7 @@ private:
     Bundle* mBundle;
     
     // key = string resource name, value = set of locales in which that name is defined
-    map<String16, set<String8> > mLocalizations;
+    map<String16, map<String8, SourcePos> > mLocalizations;
 };
 
 #endif
diff --git a/tools/aapt/SourcePos.cpp b/tools/aapt/SourcePos.cpp
index e2a921c..ae25047 100644
--- a/tools/aapt/SourcePos.cpp
+++ b/tools/aapt/SourcePos.cpp
@@ -10,17 +10,20 @@ using namespace std;
 // =============================================================================
 struct ErrorPos
 {
+    enum Level {
+        NOTE,
+        WARNING,
+        ERROR
+    };
+
     String8 file;
     int line;
     String8 error;
-    bool fatal;
+    Level level;
 
     ErrorPos();
     ErrorPos(const ErrorPos& that);
-    ErrorPos(const String8& file, int line, const String8& error, bool fatal);
-    ~ErrorPos();
-    bool operator<(const ErrorPos& rhs) const;
-    bool operator==(const ErrorPos& rhs) const;
+    ErrorPos(const String8& file, int line, const String8& error, Level level);
     ErrorPos& operator=(const ErrorPos& rhs);
 
     void print(FILE* to) const;
@@ -29,7 +32,7 @@ struct ErrorPos
 static vector<ErrorPos> g_errors;
 
 ErrorPos::ErrorPos()
-    :line(-1), fatal(false)
+    :line(-1), level(NOTE)
 {
 }
 
@@ -37,41 +40,16 @@ ErrorPos::ErrorPos(const ErrorPos& that)
     :file(that.file),
      line(that.line),
      error(that.error),
-     fatal(that.fatal)
+     level(that.level)
 {
 }
 
-ErrorPos::ErrorPos(const String8& f, int l, const String8& e, bool fat)
+ErrorPos::ErrorPos(const String8& f, int l, const String8& e, Level lev)
     :file(f),
      line(l),
      error(e),
-     fatal(fat)
-{
-}
-
-ErrorPos::~ErrorPos()
-{
-}
-
-bool
-ErrorPos::operator<(const ErrorPos& rhs) const
-{
-    if (this->file < rhs.file) return true;
-    if (this->file == rhs.file) {
-        if (this->line < rhs.line) return true;
-        if (this->line == rhs.line) {
-            if (this->error < rhs.error) return true;
-        }
-    }
-    return false;
-}
-
-bool
-ErrorPos::operator==(const ErrorPos& rhs) const
+     level(lev)
 {
-    return this->file == rhs.file
-            && this->line == rhs.line
-            && this->error == rhs.error;
 }
 
 ErrorPos&
@@ -80,18 +58,34 @@ ErrorPos::operator=(const ErrorPos& rhs)
     this->file = rhs.file;
     this->line = rhs.line;
     this->error = rhs.error;
+    this->level = rhs.level;
     return *this;
 }
 
 void
 ErrorPos::print(FILE* to) const
 {
-    const char* type = fatal ? "error:" : "warning:";
+    const char* type = "";
+    switch (level) {
+    case NOTE:
+        type = "note: ";
+        break;
+    case WARNING:
+        type = "warning: ";
+        break;
+    case ERROR:
+        type = "error: ";
+        break;
+    }
     
-    if (this->line >= 0) {
-        fprintf(to, "%s:%d: %s %s\n", this->file.string(), this->line, type, this->error.string());
+    if (!this->file.isEmpty()) {
+        if (this->line >= 0) {
+            fprintf(to, "%s:%d: %s%s\n", this->file.string(), this->line, type, this->error.string());
+        } else {
+            fprintf(to, "%s: %s%s\n", this->file.string(), type, this->error.string());
+        }
     } else {
-        fprintf(to, "%s: %s %s\n", this->file.string(), type, this->error.string());
+        fprintf(to, "%s%s\n", type, this->error.string());
     }
 }
 
@@ -116,40 +110,34 @@ SourcePos::~SourcePos()
 {
 }
 
-int
+void
 SourcePos::error(const char* fmt, ...) const
 {
-    int retval=0;
-    char buf[1024];
     va_list ap;
     va_start(ap, fmt);
-    retval = vsnprintf(buf, sizeof(buf), fmt, ap);
+    String8 msg = String8::formatV(fmt, ap);
     va_end(ap);
-    char* p = buf + retval - 1;
-    while (p > buf && *p == '\n') {
-        *p = '\0';
-        p--;
-    }
-    g_errors.push_back(ErrorPos(this->file, this->line, String8(buf), true));
-    return retval;
+    g_errors.push_back(ErrorPos(this->file, this->line, msg, ErrorPos::ERROR));
 }
 
-int
+void
 SourcePos::warning(const char* fmt, ...) const
 {
-    int retval=0;
-    char buf[1024];
     va_list ap;
     va_start(ap, fmt);
-    retval = vsnprintf(buf, sizeof(buf), fmt, ap);
+    String8 msg = String8::formatV(fmt, ap);
     va_end(ap);
-    char* p = buf + retval - 1;
-    while (p > buf && *p == '\n') {
-        *p = '\0';
-        p--;
-    }
-    ErrorPos(this->file, this->line, String8(buf), false).print(stderr);
-    return retval;
+    ErrorPos(this->file, this->line, msg, ErrorPos::WARNING).print(stderr);
+}
+
+void
+SourcePos::printf(const char* fmt, ...) const
+{
+    va_list ap;
+    va_start(ap, fmt);
+    String8 msg = String8::formatV(fmt, ap);
+    va_end(ap);
+    ErrorPos(this->file, this->line, msg, ErrorPos::NOTE).print(stderr);
 }
 
 bool
diff --git a/tools/aapt/SourcePos.h b/tools/aapt/SourcePos.h
index 33f72a9..4ce817f 100644
--- a/tools/aapt/SourcePos.h
+++ b/tools/aapt/SourcePos.h
@@ -17,8 +17,9 @@ public:
     SourcePos();
     ~SourcePos();
 
-    int error(const char* fmt, ...) const;
-    int warning(const char* fmt, ...) const;
+    void error(const char* fmt, ...) const;
+    void warning(const char* fmt, ...) const;
+    void printf(const char* fmt, ...) const;
 
     static bool hasErrors();
     static void printErrors(FILE* to);