[PATCH] MC: Support larger COFF string tables

[Richard]

================
Comment at: lib/Object/COFFObjectFile.cpp:57
@@ +56,3 @@
+// prefixed slashes.
+static bool decodeBase64StringEntry(StringRef Str, uint32_t &Result) {
+  assert(Str.size() <= 6 && "String too long, possible overflow.");
----------------
Nico Rieck wrote:
> Richard wrote:
> > This seems like something that should be trivial to unit test as it is not directly coupled to anything else and its inputs and outputs are parameters.
> > 
> > It seems odd that this function returns true on failure and false on success.
> > 
> > When reading the code aloud it sounds as "if decode base64 string entry then failed.", which seems backwards.  I'd rather see true indicating success and false indicating failure.  This does mean that you have to reverse the test in your if statements later on, but when you do they read more obviously: "if not decode base64 string entry then failed."
> If returns true on failure because this is usually used in LLVM to indicate failure. See getAsInteger() further down, or error_code. And true, these two encode/decode functions can be unit tested, but what matters is whether the string table is correctly generated, and this requires lots of code, or on-the-fly code generation.
If returning true to indicate failure is the convention in this code base, then leave that as is.  Even if it does strike me as backwards! :-)


http://llvm-reviews.chandlerc.com/D667

BRANCH
  arc/667-coff-string-table-v3

ARCANIST PROJECT
  llvm