[PATCH] D150987: [llvm-nm] Add --line-numbers flag

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:1-2
+# Check that printing line numbers is not attempted for symbolic object files
+# that are not object files.
+# RUN: llvm-nm --line-numbers %p/Inputs/test.IRobj-x86_64 | FileCheck %s --strict-whitespace --check-prefix=BITCODE
----------------
jhenderson wrote:
> Nit: new tests in many of the LLVM tools use `##` for comment markers, to help them stand out from the lit and FileCheck directives.
> 
> I don't understand how a "symbolic object file" is not already an "object file". This comment looks suspiciously like it's essentially copying code terms into the English prose, without actually stating what is significant. What is a "symbolic object file"? What is an "object file" in this context? Most importantly, how are the two different?
Yep, that's the case, but it's a good point that the tests should be readable while treating the implementation as a black box.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:42-44
+# Check that printing line numbers for undefined values does not include
+# relocations for non-text sections. This is broken out of main.s to ensure
+# that the data relocation for undef comes first.
----------------
jhenderson wrote:
> Nit: I'm confused what line wrapping rules you're following in this test file - compare this to the previous comment, and you'll note that one is not wrapped at all, whereas this one is broken up over three lines.
Yeah, my intent was to break comments along an 80 column boundary, but this was inconsistently applied.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:697
+    return;
+  SymbolRef SR = S.Sym;
+  uint64_t SectionIndex = object::SectionedAddress::UndefSection;
----------------
jhenderson wrote:
> mysterymath wrote:
> > jhenderson wrote:
> > > I'm not convinced there's any value in this local variable. In fact, the name `SR` is non-obvious as to its purpose or anything, and this function is somewhat long. I'd just get rid of the local variable entirely.
> > This performs a cast; I've rewritten this to explicitly call the constructor to make this clearer. Renamed to `Sym`.
> Could `const SymbolRef Sym(S.Sym);` be used? That seems clearer to me.
Yeah, that reads better to me too now that you mention it. Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150987