[PATCH] D112450: support xcoff for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:131
 
+static void warning(Error Err, StringRef Input) {
+  assert(Err);
----------------
`warn` not `warning` - `warn` is a verb, `warning` is a noun, and functions should be verbs or verb phrases.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:927
+  if (!TypeOrErr) {
+    warning(TypeOrErr.takeError(), Obj.getFileName());
+    return '?';
----------------
Test case?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:941
+  if (!SymSecOrErr) {
+    warning(SymSecOrErr.takeError(), Obj.getFileName());
+    return '?';
----------------
If possible, add additional context to this error: which symbol has the invalid section?

Something like the following message would be ideal:

"warning: "a.o": unable to load section for symbol "foo": ..."
or
"warning: "a.o": unable to load section for symbol with index 3: ..."

Same goes above.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:144
 
+void reportWarning(Error Err, StringRef Input) {
+  assert(Err);
----------------
DiggerLin wrote:
> jhenderson wrote:
> > # Move this function so that it isn't sandwiched by the `error` functions.
> > # I think you can just use `WithColor::defaultWarningHandler` to replace most of the logic around `handleAllErrors` below.
> > # For consistency with the `error` functions in this file, I'd just call this function `warn` rather than `reportWarning`.
> 
> if using WithColor::defaultWarningHandler(Error Warning)  directly , we  can not tell user which object file has the warning. 
> 
> 
That's not true: just pass in an error created via createFileError, right? The implementation of the defaultWarningHandler is almost identical to your existing code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112450