[PATCH] D120357: [llvm-nm][refactor] add helper function to print out the object file name, archive name, architecture name

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Please don't mark a patch as a refactor when there are behaviour changes: by definition "refactor" means a change with no change in behaviour.

Please don't change the output of llvm-nm, at least for regular ELF output and probably for all foramts, unless GNU nm output has changed. The LLVM tools like llvm-nm, llvm-readelf, llvm-objdump are supposed to produce pretty much identical output to the GNU utilities, so that they can be used as drop-in replacements. The version of GNU nm on my machine prints archive member names as `member.o:` not `archive.a(member.o):` or similar. I do agree that the proposed output is better, but we can't break this requirement, unless there's buy-in from the GNU community, as lots of people rely on automated scripts to parse the tool output.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1865-1866
+
+static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool PrintName,
+                                      bool PrintObjectNameInfo,
+                                      StringRef ArchiveName = {},
----------------
It's not clear what the difference is between `PrintName` and `PrintObjectNameInfo` from just the variable names. I suggest renaming `PrintName` to be more specific (I think it's `PrintSymbolName` right?). Also, I'd drop "Info" from the `PrintObjectNameInfo` variable name.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2199
     return;
-  dumpSymbolNamesFromObject(*O, true);
+  dumpSymbolNamesFromObject(*O, true, false);
 }
----------------
Label these booleans with comments, i.e. as per the inline edit.

Same goes at the other call sites.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120357