[PATCH] D84526: [yaml2obj] - Add a support for "<none>" value for all optional fields.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: Higuoxing.
jhenderson added a comment.

What's the motivation for testing every `Optional<T>` type? It seems to me like this code is a common piece of code where a single example would be sufficient. In fact, this test doesn't include coverage for the DWARF tag, for example (see @Higuoxing's recent work), which has many optional fields.



================
Comment at: llvm/test/tools/yaml2obj/ELF/none-value.yaml:3
+## as Optional<> in the code. Setting a key to "<none>" means no-op and
+## works in the same way as when a value was not set at all.
+
----------------
Perhaps change "value was not set" to "field was not specified"? I think it avoids ambiguity of the term "value", which might be the thing on the right of the colon.


================
Comment at: llvm/test/tools/yaml2obj/ELF/none-value.yaml:10
+
+## We do not use the TEST macro. It is exist to
+## demonstrate the expected use case for the <none> word.
----------------
It is exist -> It exists


================
Comment at: llvm/test/tools/yaml2obj/ELF/none-value.yaml:182
+    Type: SHT_DYNAMIC
+## Note: for SHT_NOTE sections, one of "Content", "Size" or "Notes" must be specified.
+  - Name: .note.1
----------------
You can probably get rid of the notes from the second YAML. The initial comment saying it is a duplicate, barring the missing fields, is sufficient to convey that, I think.


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

https://reviews.llvm.org/D84526