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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:4
+
+namespace llvm {
+
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > I suggest to remove this and qualify the function definition explicitly.
> I am not sure whether we will add new function in the file later for the new functionality of the llvm-objdump, if we remove the namespace llvm , we have to add the llvm qualify for very function and parameter. I am refer to keep this way.
If the function name is qualified, the parameter types do not need to be. The rationale for qualifying the function name (low cost for the benefit, IMO) was given below.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:21
+  }
+  return XCOFF::XMC_MAX_SMC;
+}
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > Should we just return an `int` and use `-1` as a magic number? `XMC_MAX_SMC` does not really capture the semantics. It's the largest value that fits, but it is not currently valid. It is also not guaranteed not to become a valid value in the future. The `-1` magic number is more idiomatic here.
> the getXCoffSymbolCsectSMC() will be put in the third element of  SymbolInfoTy = std::tuple<uint64_t, StringRef, uint8_t, int64_t>; , it is unsigned 8bits
Just include the type (or the "is a label" property) into the `SymbolInfoTy` alongside the real SMC value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973