[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:142-143
+        WithColor::warning(errs(), ToolName)
+            << EI.message() << " for symbol(index:" << SymIdx << ")."
+            << "\n";
+      });
----------------
As a general note, please remember in English prose (such as warning messages) to have a space before opening parentheses. However, in this particular case, I'd suggest a slight restructuring of the message, which will work regardless of the output of `EI.message()`. See the inline edit.

This puts the symbol index before the rest of the message. In fact, I'd recommend making it even more generic, and change the signature to `static void warn(Error Err, Twine Path, Twine Context = Twine())`, with it becoming something like: 
```
WithColor::warning(errs(), ToolName) << Path << ": " << (Context.empty() ? "" : Context + ": ")
    << EI.message() << "\n";
```
The reason is that this is a generic warning method, not "warnForSymbolError" or similar. If you'd like to add an additional wrapper function that converts a symbol index into a context string, before calling this, that would be fine too. The reasons for the suggested changes:
1) The `error` functions take a `Twine Path` for the file name, so change this to be consistent with those.
2) There may be occasions in the future of llvm-nm when we want to print a warning where no additional context is needed, so handle that case too.
3) Use `Twine` rather than `StringRef` for the Context string, to allow it to be built up dynamically (note that the `error` function that takes a Message rather than an Error uses Twine for the Message).

Also, as a reminder, modern LLVM style doesn't have full stops at ends of errors and warnings.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:942-943
+
+  // If the I->getSection() return error, the I->getType() will return same
+  // error first, it will not come to here.
+  section_iterator SecIter = cantFail(I->getSection());
----------------
Some suggested improvements. Please reflow too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112450