[PATCH] D61180: [yaml2obj] - Allow setting st_value explicitly for Symbol.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Some comment nits, otherwise LGTM.



================
Comment at: test/tools/yaml2obj/symbol-name.yaml:1
+# Check we are able to use integers as both
+# symbol name indices (st_name values) and symbol names.
----------------
FWIW, I liked what you did in other tests with the double '#' symbols to indicate comments. I wouldn't object to you doing it here either.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:472
+
+    // If name index containing offset is explicitly specified in YAML, we use
+    // it. It is useful for preparing broken objects. Otherwise, we add a Name
----------------
name index containing offset -> NameIndex, which contains the name offset,
remove "in YAML", since theoretically, we could use this bit of code via other methods in the future.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:473
+    // If name index containing offset is explicitly specified in YAML, we use
+    // it. It is useful for preparing broken objects. Otherwise, we add a Name
+    // to the string table builder to get its offset.
----------------
It is -> This is
a Name -> the specified Name


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

https://reviews.llvm.org/D61180