[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 8 inline comments as done.
DiggerLin added a comment.

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

>> I do not think we need test for a empty line of the first object file of the MachOUniversalBinary.
>
> It is important to test whitespace as much as non-whitespace, as we don't want regressions in how things are printed (e.g. double blank lines, no blank lines when expected etc). This can make the output look messy, apart from anything else.
>
> Having looked at the output, I agree that having a blank line at the start makes sense, but I want to be sure we enshrine this in a test.

I can not find  macho-universal-archive which only have  a single object file in the llvm/test/Object/Inputs, and I am sorry that I do not have MacOS env to create a macho-universal-archive which have only a single object file for test purpose.
 Anther solution is that  I will add a new parameter for function dumpSymbolNamesFromObject() (if I add a new parameter , is  there too much parameter for the function?)  to control whether print out the "\n"  in printObjectLabel() and change the patch to NFC patch. What is your opinion ?

> Tip: when posting comments including code blocks or tool output etc, use the "Code Block" formatting option (left of the quote button in the Phabricator comment toolbar). The markup is three "`" characters for a multi-line output, or just one for inline content, at each end of the block.





================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1973
+      dumpSymbolNamesFromObject(*O, /*PrintSymbolObject=*/false,
+                                /*PrintObjectLabel=*/!PrintFileName, Filename,
+                                /*ArchitectureName=*/{}, O->getFileName(),
----------------
jhenderson wrote:
> As already requested...
from the '!PrintFileName" , we can not know it is  the value of parameter of  "PrintObjectLabel" at first glance , so I keep the comment here on purpose. I delete it anyway.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1851
 
-  if (ExportSymbols)
+static void printObjectNames(bool isArchive, StringRef ArchiveName,
+                             StringRef ArchitectureName,
----------------
jhenderson wrote:
> DiggerLin wrote:
> > 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.
> `IsArchiveFile` is no better: it has the same problems:
> 1) The input file for this function is not an archvie. It's an object (which may be part of an archive).
> 2) The code path for `!IsArchiveFile` is only relevant when the input file IS a member of an archive, which is contradictory to the name of the parameter.
> How about `PrintArchiveName` (and flipping its logic).
OK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120357