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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

A few comments/suggestions from me.



================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml:13
+    Type:            STT_FILE
+    Index:           SHN_ABS
+
----------------
nit: we often remove the excessive identation. It
makes the output to be a bit more readable.


================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml:15
+
+# When a file contains only local symbols the "no symbols" error should not
+# be shown, so we expect the output to be completely empty.
----------------
nit: Some of the llvm-nm tests are using ## for comments, just like
many other our tools already do. I'd follow this style as it is really common nowadays.


================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols.test:11
 
-# CHECK: {{^}}no symbols{{$}}
-
-# CHECK-PRINT-FILE-NAME: nm-no-symbols.test{{.*}}.o: no symbols{{$}}
+# CHECK: nm-no-symbols.test{{.*}}.o: no symbols{{$}}
----------------
I see that this way of name checking existed before this patch, but FTR my concern
is that the output name depends on the lit naming rules.

I.e. I'd suggest to be a bit more explicit here:

```
# RUN: yaml2obj %s > %t.o
# RUN: llvm-nm %t.o 2>&1 | FileCheck %s -DFILE=%t.o
...
# CHECK: [[FILE]]: no symbols{{$}}
```




================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:710
 
+void writeFileName(raw_ostream &S, const std::string &ArchiveName,
+                   const std::string &ArchitectureName) {
----------------
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.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1740
+    writeFileName(errs(), ArchiveName, ArchitectureName);
+    errs() << "no symbols\n";
+  }
----------------
You probably can use the existent `error()` method from line 224 for reporting:

```
static void error(Twine Message, Twine Path = Twine()) {
  HadError = true;
  WithColor::error(errs(), ToolName) << Path << ": " << Message << ".\n";
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72658