[PATCH] D71872: [yaml2obj/obj2yaml] - Add support for SHT_RELR sections.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:3
+
+## Test that the content of SHT_RELR sections for 64-bits little endian targets is correct.
+# RUN: yaml2obj --docnum=1 %s -o %t.le64
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > 64-bits -> 64-bit
> > > 
> > > (a target is described as being a 64/32-bit target - other places 64-bits would be correct).
> > (I hope I got it right)
> > 
> > Fixed here and in 3 more similar places in this test.
> > I've kept other comments, particularry one in `obj2yaml/relr-section.yaml`
> > ("Test how we dump SHT_RELR sections for 32 and 64-bits targets.") unchanged.
> After thinking about it yesterday, I think I figured out the rule. When 32/64-bit is used as an adjective followed by a noun (e.g. "This is a 64-bit target"), it shouldn't have the 's', whereas when it is used without a following noun (e.g. "This target is 64-bits"), it should.
> 
> Aside from the obj2yaml/relr-section.yaml case (which should be updated), I couldn't find any other 64-bits usage in comments in this patch. Which were the others you were referring to?
> After thinking about it yesterday, I think I figured out the rule. When 32/64-bit is used as an adjective followed by a noun (e.g. "This is a 64-bit target"), it shouldn't have the 's', whereas when it is used without a following noun (e.g. "This target is 64-bits"), it should.

Interesting. I am usually simply thinking about "s" as about suffix for a plural form of a noun. In Russian the correct spelling for a simple straightforward translation of "64-bit/64-bits" would be different for "target" and "targets" cases. 
(In the "Test that the content of SHT_RELR sections for 64-bit little endian targets is correct." sentence I mean).
That is where my confuson here comes from I think.

> Aside from the obj2yaml/relr-section.yaml case (which should be updated), I couldn't find any other 64-bits usage in comments in this patch. Which were the others you were referring to?

Nope. I wasn't sure what you meant by "other places" and only tried to point to places I found which I did (or did not) change :)


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

https://reviews.llvm.org/D71872