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

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

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(),
----------------
As already requested...


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1975
+                                /*ArchitectureName=*/{}, O->getFileName(),
+                                /*IsArchiveFile*/ true);
     }
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2005
+              Obj, /*PrintSymbolObject=*/false,
+              /*PrintObjectLabel=*/(ArchFlags.size() > 1) && !PrintFileName,
+              ArchiveName, ArchitectureName);
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2034
+              dumpSymbolNamesFromObject(*O, /*PrintSymbolObject=*/false,
+                                        /*PrintObjectLabel=*/!PrintFileName,
+                                        ArchiveName, ArchitectureName);
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2091
+            dumpSymbolNamesFromObject(*O, /*PrintSymbolObject=*/false,
+                                      /*PrintObjectLabel=*/!PrintFileName,
+                                      ArchiveName);
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2125
+      dumpSymbolNamesFromObject(Obj, /*PrintSymbolObject=*/false,
+                                /*PrintObjectLabel=*/!PrintFileName,
+                                ArchiveName, ArchitectureName);
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2148
+          dumpSymbolNamesFromObject(*F, /*PrintSymbolObject=*/false,
+                                    /*PrintObjectLabel=*/!PrintFileName,
+                                    ArchiveName, ArchitectureName);
----------------



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120357