[PATCH] D61551: [llvm-nm] Fix handling of symbol types 't' 'd' 'r'

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 6 inline comments as done.
MaskRay added inline comments.


================
Comment at: test/tools/llvm-nm/linker-synthesized.test:4
+
+# We used to be wrong with some linker synthesized symbols.
+
----------------
jhenderson wrote:
> I think it would be better if the comment is at the top of the file.
> 
> Up to you, but I've also seen some people recently using '##' to clearly indicate comments versus lit/FileCheck stuff. It might be nice if you add them, but I don't mind if you don't want to.
> 
> If you've got a reference to a reported bug, it would be great to add that.
`##` is a great idea!  It highlights the comments part.

> I think it would be better if the comment is at the top of the file.

Perhaps I'm too used to lld tests and there these comments are usually below the RUN lines... I know comments before RUN lines can be regarded as file comments while the comments above may describe the individual tests. In practice, I think there isn't a dichotomy of these two types of comments.  I'm ok with either one but inclined to keep it unchanged unless you have strong opinion on this.


================
Comment at: test/tools/llvm-nm/nobits.test:22
+Symbols:
+  - Name:    mybss_local
+    Binding: STB_LOCAL
----------------
jhenderson wrote:
> Perhaps for completeness sake, it's worth adding a local TLS symbol too.
I was on the fence about the choice. Since you suggest we go for completeness, I'll do that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61551