[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 marked 2 inline comments as done.
hubert.reinterpretcast added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:17
+namespace objdump {
+Optional<XCOFF::StorageMappingClass>
+getXCOFFSymbolCsectSMC(const object::XCOFFObjectFile *Obj,
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > Please include an interface header that is expected to provide `Optional` as part of its interface.
> > The comment is not actually addressed. `XCOFFObjectFile.h` does not physically contain any reference to `Optional`.
> I'm not sure this is necessary? According to https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible it's okay to go via other headers to include things. The route to ADT/Optional.h is:
> 
> Object/XCOFFObjectFile.h
> BinaryFormat/XCOFF.h
> ADT/StringRef.h
> ADT/STLExtras.h
> ADT/Optional.h
> 
> (From a philosophical point of view, I don't actually agree with this part of the coding standards - when coding on other projects, I try to include the headers directly that define the things I need - but we should adhere to LLVM style).
Okay. Thanks.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:24
+void printXCOFFSymbolDescription(const SymbolInfoTy &SymbolInfo,
+                                 StringRef SymbolName);
+
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > Same comment about interface header but for `StringRef`.
> Ditto response.
Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973