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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> in the function dumpMachOUniversalBinaryArchAll , delete the functionality:

Your commit message indicates that there's a functional change, but there's no corresponding test change. That implies there's either a missing test, or you've broken an existing test. I suggest adding a test to cover the output difference.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1851
 
-  if (ExportSymbols)
+static void printObjectNames(bool isArchive, StringRef ArchiveName,
+                             StringRef ArchitectureName,
----------------
Please remember UpperCamelCase for variable names.

That being said, I'm not convinced `IsArchive` is especially meaningful or even correct, given that the `false` case is also an archive member sometimes. I suggest just passing in an empty string for `ArchiveName` if the archive name shouldn't be printed.

You're also only printing one object, so I recommend against calling it `printObjectNames` (plural). Even though you are printing multiple names, there is only one name for the object; the rest just provide additional context.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2198-2199
+                                /*ArchiveName=*/{},
+                                /*ArchitectureName=*/ArchName,
+                                /*ObjectName=*/I.getInstallName());
+    else if (Error E = isNotObjectErrorInvalidFileType(ObjOrErr.takeError())) {
----------------
Here and throughout, there's no need to label the parameters with comments if the input makes it somewhat obvious what they may represent. I personally use it only for literals like `true`, `false`, `0`, `1` and `{}` etc. In other words, remove the last two labels here and anywhere where a variable is being passed in, rather than a literal.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1864
+
+static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool PrintName,
+                                      bool PrintObjectNameInfo,
----------------
DiggerLin wrote:
> MaskRay wrote:
> > Try `const SymbolicFile &Obj` and fix some non-const references in the body.
> Agree with you , but if I change the to const SymbolicFile &Obj, a lot of functions's parameter need to "const SymbolicFile &Obj" ,
> for example, printSymbolList() ,isSymbolList64Bit() , I  am prefer to keep as it is now. and another separate patch for it. if you strong suggest that we need to change it the patch. I can do it.
+1 to leaving it for now, but fixing in a separate patch, since the situation is no worse than it was before.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1865-1866
+
+static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool PrintName,
+                                      bool PrintObjectNameInfo,
+                                      StringRef ArchiveName = {},
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > 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.
> > it is not printing symbol name. it print object file name relate info. I thin PrintName is too generic. I used PrintObjectName?
> sorry for misunderstand comment, please ignore above comment.
> 1. I changed PrintName  to PrintSymObjectName
> 2. changed PrintObjectNameInfo to PrintObjectName
I'm still not a massive fan of the variable names, as they are still somewhat unclear. How about `PrintSymbolObject` and `PrintObjectLabel` respectively?

I use "Label" in this context, because it's similar to assembly or C labels, so should be familiar enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120357