[PATCH] D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:769
 
+// TODO: The function need to return an error if there is no Csect auxiliary
+// entry.
----------------
DiggerLin wrote:
> jhenderson wrote:
> > need -> needs (same below)
> > 
> > Is there any reason you can't do this in a separate and prerequisite commit?
> yes, we can do it separate and prerequisite commit. but We prefer to  have a NFC plan for it after this patch . @hubert.reinterpretcast  
The change indicated by the TODO might be easier to understand and tune with the context added by the additional uses from this patch. Adding it to this patch would go past the intended scope of this patch to include adjustments needed for some existing calls. I think a follow-on patch makes sense.

I do understand that there have been multiple things identified as improvements that we want through the review of this patch. In terms of improvements to existing code quality, we are working on D77285, and the subject of the TODO here is the other code quality (as opposed to, e.g., functional behaviour) follow-on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973