[PATCH] D120679: [Object] [COFF] Improve error messages

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the improvement. Whilst I get what @rnk's saying about testing, I've learnt from past experience that if an error message is not tested, there's a non-trivial chance that the message is garbled in some way, e.g. due to incorrect format strings, or using wrong parameters. I'd personally prefer seeing a test for each case that has the additional new context (including, where possible, using the actual values passed in, rather than regexes), at least if there's already testing for the error path.



================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:488
+    return createStringError(object_error::parse_failed,
+                             "RVA %" PRIx32 " for %s not found", Addr,
+                             ErrorContext);
----------------
Here and below, do you want the "0x" prefix?


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:490
+                             ErrorContext);
+  else
+    return createStringError(object_error::parse_failed,
----------------
No need for `else` after `return`.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:517
+                             ErrorContext);
+  else
+    return createStringError(object_error::parse_failed,
----------------
No need for `else` after `return`.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/tls-directory.test:107
 
-# BAD-TLS-RVA-ERR: error: '{{.*}}': Invalid data was encountered while parsing the file
+# BAD-TLS-RVA-ERR: error: '{{.*}}': RVA {{.*}} for TLS directory not found
 
----------------
For the filename parameter, I'd consider passing in the filename as a FileCheck variable, although I acknowledge it didn't before, so am not too fussed by this.

For the RVA parameter, if you don't want to hard-code in the value, I'd use a FileCheck numeric variable to ensure that the value is actually an expected number. A possible style is in the inline edit, but you may want a tighter restriction etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120679/new/

https://reviews.llvm.org/D120679