[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
llvm-commits at lists.llvm.org
Mon Feb 28 02:45:19 PST 2022
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
More information about the llvm-commits
mailing list