[PATCH] D90458: [yaml2obj] - ProgramHeaders: introduce FirstSec/LastSec instead of Sections list.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:493
+                  Twine(I));
+    size_t Last = YamlPhdr.LastSec ? NameToIndex[*YamlPhdr.LastSec] : First;
+    if (!Last && YamlPhdr.LastSec)
----------------
jhenderson wrote:
> This behaviour isn't immediately clear to me as being the behaviour when just `FirstSec` is specified. It could easily be thought that just `FirstSec` implies that section and all later sections. What do you think about requiring both keys if either is specified?
> It could easily be thought that just FirstSec implies that section and all later sections.

I had such concern too. Was not sure what is better. I've made both keys to be required if either is specified.


================
Comment at: llvm/test/tools/obj2yaml/ELF/program-headers.yaml:331-332
 
 ## Test we include non-allocatable sections in segments.
 ## We also document that SHT_NULL sections are not considered to be inside a segment.
 # RUN: yaml2obj --docnum=4 %s -o %t4
----------------
jhenderson wrote:
> This comment probably needs updating.
I've updated the test itself instead.


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

https://reviews.llvm.org/D90458