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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 2 inline comments as done.
DiggerLin added a comment.

In D120357#3348530 <https://reviews.llvm.org/D120357#3348530>, @jhenderson wrote:

>> 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.



In D120357#3348530 <https://reviews.llvm.org/D120357#3348530>, @jhenderson wrote:

>> 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.

In original source code 
if (moreThanOneArch)

outs() << "\n";
if the MachOUniversalBinary has more than one object , it print as "

"
(there is one empty line here)
a.o:

XXXXXX

b.o:

XXXXXX"

if only have one object file , it printed as "
"
a.o:
XXXXXXX
"

There is no test to test the whether there is  no empty line before the first object file of the MachOUniversalBinary  if the only one object file in the MachOUniversalBinary

I change to always print out

  "

(there is one empty line here)
a.o:

XXXXXX
"  
for the first object file of MachOUniversalBinary  , it is consistency with other part of the code.

I do not think we need to test it now.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1851
 
-  if (ExportSymbols)
+static void printObjectNames(bool isArchive, StringRef ArchiveName,
+                             StringRef ArchitectureName,
----------------
jhenderson wrote:
> 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.
> 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 changed to IsArchiveFile.


> I suggest just passing in an empty string for ArchiveName if the archive name shouldn't be printed.

I do not think we can set the ArchiveName to empty if archive name should not print in the printObjectName , for the function  writeFileName(errs(), ArchiveName, ArchitectureName); and  printSymbolList(Obj, PrintSymObjectName, ArchiveName, ArchitectureName) still need it .
in the  dumpSymbolNamesFromObject.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2126
-        if (moreThanOneArch)
-          outs() << "\n";
-        outs() << Obj.getFileName();
----------------
I think it is bug of the code,

if (moreThanOneArch)
          outs() << "\n";

if the MachOUniversalBinary has more than one object , it print as "

"
(there is one empty line here)
a.o:

XXXXXX

b.o:

XXXXXX"

if only have one object file , it printed as 

"a.o:    //(there is no empty before the a.o)
XXXXXXX
"

it is not consistency for first object file a.o (whether there is a empty line before the first object file name(or Lable)"



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1865-1866
+
+static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool PrintName,
+                                      bool PrintObjectNameInfo,
+                                      StringRef ArchiveName = {},
----------------
jhenderson wrote:
> 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.
OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120357