[PATCH] D71735: [ELF] Don't suggest an alternative spelling for a symbol in a discarded section

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 23 01:01:34 PST 2019


grimar accepted this revision.
grimar added a comment.

LGTM with a few nits/suggestions.

Perhaps the test name should be `*.test` instead of `*.yaml`. This is consistent with other our tests that use YAML (ELF\invalid\*).
(we have only one *.yaml test, gdb-index-invalid-section-index.yaml, seems it was named inconsistently)



================
Comment at: lld/test/ELF/undef-not-suggest.yaml:34
+    Relocations:
+      ## Relocation which references .text.foo . Check we don't suggest .data,
+      ## which has an empty name.
----------------
"Check we don't suggest .data which has an empty name."
sounds like `.data` has an empty name, what can't be true.
Perhaps, `check we don't suggest the section symbol for ".data", which ...` ?

Also, `.text.foo .` -> `".text.foo".` ? (to get rid of trailing space after the section name).


================
Comment at: lld/test/ELF/undef-not-suggest.yaml:40
+        Type:   R_X86_64_64
+      ## Relocation which references foo. Check we don't suggest for.
+      - Offset: 0x8
----------------
`foo->"foo"`, `for -> "for"`? (because "Check we don't suggest for" looks like an incomplete sentence probably).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71735





More information about the llvm-commits mailing list