[PATCH] D55220: [yaml2obj] Move redundant statements into a separate static function

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added inline comments.
This revision now requires changes to proceed.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:229
 
+static bool checkSectionIndex(NameToIdxMap &SX2I, StringRef SecName,
+                              StringRef SecField, unsigned &Index) {
----------------
"check" implies to me that it's just validating something. However, this function does more than that (it sets the Index variable too). I'd suggest renaming the function `convertSectionIndex` or similar.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:230
+static bool checkSectionIndex(NameToIdxMap &SX2I, StringRef SecName,
+                              StringRef SecField, unsigned &Index) {
+  if (SX2I.lookup(SecField, Index) && !to_integer(SecField, Index)) {
----------------
`SecField` to me implies the name of a section field, e.g. "sh_link". I'd suggest renaming `SecField` to `IndexSrc` or similar, and `Index` to `IndexDest`. The two are closely related, so having similar names makes sense to me.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:278
       unsigned SymIdx;
-      if (SymN2I.lookup(S->Info, SymIdx) && !to_integer(S->Info, SymIdx)) {
-        WithColor::error() << "Unknown symbol referenced: '" << S->Info
-                           << "' at YAML section '" << S->Name << "'.\n";
+      if (!checkSectionIndex(SymN2I, S->Name, S->Info, SymIdx))
         return false;
----------------
This isn't a section index lookup. It's a symbol lookup. Note that the message is different.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55220