[PATCH] D72658: [llvm-nm] Don't report "no symbols" error for files that contain symbols

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml:1
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-nm %t.o 2>&1 | FileCheck --allow-empty %s
----------------
jhenderson wrote:
> It migth make more sense to merge this and the existing test into a single test, with multiple YAML documents, since it's only a minor variation.
I'd rather leave it separate if thats ok with you.    They seems different enough to me and it makes it easier to run them individually.


================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml:2
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-nm %t.o 2>&1 | FileCheck --allow-empty %s
+
----------------
jhenderson wrote:
> I know this existed in the pre-existing test, but I feel like that test should be fixed too. This won't prove that "no symbols" is printed on stderr, because it's merged with stdout. I think it would be best to pipe stderr to a separate file completely and check that file.
I'm just trying to show the output is empty.   I've updated it to use "count 0" which clearer.

If you want to fix the existing test thats a separate issue.


================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml:13
+    Type:            STT_FILE
+    Index:           SHN_ABS
+
----------------
grimar wrote:
> nit: we often remove the excessive identation. It
> makes the output to be a bit more readable.
The prevailing style in this directory seems to be to preserve the yaml with indentation and alignment.  I'm not sure I agree that its more readable either.    So if its OK with you I'll leave this as is for this change and you and propose an widespread change if you feel strongly.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:710
 
+void writeFileName(raw_ostream &S, const std::string &ArchiveName,
+                   const std::string &ArchitectureName) {
----------------
grimar wrote:
> grimar wrote:
> > I think this either could accept Twines:
> > 
> > `writeFileName(raw_ostream &S, const Twine ArchiveName, ...`
> > 
> > or I am thinking it could probably be:
> > 
> > `std::string getFileName(const Twine ArchiveName, ...)`
> > 
> > I think in the latter case the callers code can be a bit nicer/simpler probably.
> > 
> `const Twine` -> `const Twine &`
I was initially going to do that are part of this change but I'm inclined to follow the style of writeFileName below and leave that refactor to a follow up change. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72658