[PATCH] D89049: [AIX][XCOFF] print out the traceback info

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:164-169
+    outs().flush();
+    WithColor::warning(errs(), "")
+        << " Parse traceback table failure(The value in error msg is "
+           "calculated based on traceback begin) \n"
+        << "\t" << toString(TTOrErr.takeError()) << " from traceback begin."
+        << "\n\tThe raw data of traceback table as : \n";
----------------
Use `objdump::reportWarning` rather than printing to the warning stream directly, I recommend. It does the flushing and printing to the `errs()` stream itself.

If for some reason, you want to deliberately differ to what llvm-objdump does normally when printing warnings and not print the tool name, you could add an optional argument to `reportWarning` to suppress it.

The warning message you are printing itself needs a number of grammar fixes, but I'm not familiar enough with XCOFF terminology to have a reasonable recommendation. Perhaps the following (note included whitespace and capitalization fixes in the message):

`Twine("traceback table parsing failure\n>>> ") + toString(TTOrErr.takeError()) + " from the start of the traceback table.\n>>> Raw traceback table data is:"`

I'm hesitant about including the "from the start of the traceback table" bit, because it bakes in an assumption about what the underlying error message will look like, which may no longer apply in the future if the message changes. Personally, I'm not sure it's needed, and would be inclined to remove it.

Rather than using hard tabs in the output, I've followed an approach taken from LLD's error message styling too. Hard tabs are bad and should be avoided in my opinion, because the output will appear different depending on people's window settings.

I've omitted the "The value in..." bit, as I'm really not sure what value it's referring to here, and I suspect a user might get more confused by it than not, but if you want to include it, I'd add it as a note after the first line, saying "note: the value in ...".

Note also that `reportWarning` includes a trailing new line, so there is no need to explicitly add one, just as it also does `outs().flush()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049