From a01a9374fd386f3a8773528d7a49bc5315492dff Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Thu, 20 Mar 2014 18:04:57 -0700 Subject: Reduce warning verbosity in aapt - Attributed source of problems to the correct file. - Only verify string localizations against valid locales. Bug:13140015 Change-Id: I9dabc5efa0510649caee8af0c8ebb803d6f48269 --- tools/aapt/ResourceTable.cpp | 73 ++++++++++++++++++----------- tools/aapt/ResourceTable.h | 4 +- tools/aapt/SourcePos.cpp | 108 +++++++++++++++++++------------------------ tools/aapt/SourcePos.h | 5 +- 4 files changed, 98 insertions(+), 92 deletions(-) (limited to 'tools') 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& 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 >::iterator nameIter = mLocalizations.begin(); + for (map >::iterator nameIter = mLocalizations.begin(); nameIter != mLocalizations.end(); nameIter++) { - const set& configSet = nameIter->second; // naming convenience + const map& 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::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::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 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::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& 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& dest); @@ -551,7 +551,7 @@ private: Bundle* mBundle; // key = string resource name, value = set of locales in which that name is defined - map > mLocalizations; + map > 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 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); -- cgit v1.1