[PATCH] D39582: Add ELF dynamic symbol support to yaml2obj/obj2yaml

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

I'm happy with https://reviews.llvm.org/D39749 and using that to resolve the ordering isue. I'm not a reviewer here but this looks good to me other than the ternary operator nit.



================
Comment at: tools/yaml2obj/yaml2elf.cpp:301
   zero(SHeader);
-  SHeader.sh_name = DotShStrtab.getOffset(".symtab");
-  SHeader.sh_type = ELF::SHT_SYMTAB;
-  SHeader.sh_link = getDotStrTabSecNo();
+  bool IsStatic = STType == SymtabType::Static ? true : false;
+  SHeader.sh_name = DotShStrtab.getOffset(IsStatic ? ".symtab" : ".dynsym");
----------------
kastiglione wrote:
> jakehehrlich wrote:
> > Can this just be "bool IsStatic = STType == SymtabType::Static"? Better yet do we really need SymtabType? What if we just pass "IsStatic" as a parameter?
> 🤦‍♂️
> 
> The choice to use the enum was to avoid opaque `true`/`false` arguments at the call site. I'll go with bools if that's preferred.
I'm not sure I have a preference on bools vs enums, you pick, but the ternary operator use seems funny to me.


https://reviews.llvm.org/D39582