[PATCH] D66418: [llvm-objdump] - Cleanup the error reporting.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:381
 LLVM_ATTRIBUTE_NORETURN void report_error(StringRef File, Twine Message) {
-  WithColor::error(errs(), ToolName)
-      << "'" << File << "': " << Message << ".\n";
+  if (File.empty())
+    WithColor::error(errs(), ToolName) << Message << ".\n";
----------------
Keeping a separate `LLVM_ATTRIBUTE_NORETURN void report_error(Twine Message);` overload for the error message without a filename might be clearer. What do you think?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:447
+                    "found in any input file",
+                "" /*FileName*/);
 }
----------------
`/*FileName=*/""`

See above. Keeping a separate `LLVM_ATTRIBUTE_NORETURN void report_error(Twine Message);` might be better.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:450
 
-static const Target *getTarget(const ObjectFile *Obj = nullptr) {
+static const Target *getTarget(const ObjectFile *Obj) {
   // Figure out the target triple.
----------------
```
- static const Target *getTarget(const ObjectFile *Obj = nullptr) {
+ static const Target *getTarget(const ObjectFile *Obj) {
```

Nice!


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2220
   if (StartAddress >= StopAddress)
-    error("start address should be less than stop address");
+    report_error("" /*FileName*/,
+                 "start address should be less than stop address");
----------------
`/*FileName=*/""` is probably more common. (If you add a space, clang-format will delete it for you.)

See above. Keeping a separate `LLVM_ATTRIBUTE_NORETURN void report_error(Twine Message);` might be better.


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

https://reviews.llvm.org/D66418