[PATCH] D56294: [ObjectYAML] [COFF] Support multiple symbols with the same name

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I think a composite test (or tests) would makes sense, but check the intermediate output as well as the final output.



================
Comment at: include/llvm/ObjectYAML/COFFYAML.h:62
   StringRef SymbolName;
+  Optional<uint32_t> SymbolTableIndex;
 };
----------------
SymbolTableIndex -> SymbolIndex

"SymbolTableIndex" to me implies the section index of the symbol table.


================
Comment at: tools/obj2yaml/coff2yaml.cpp:146
 
+  StringMap<size_t> SymbolOccurrances;
+  for (const auto &S : Obj.symbols()) {
----------------
SymbolOccurrances -> SymbolOccurrences


================
Comment at: tools/yaml2obj/yaml2coff.cpp:531-533
       uint32_t SymbolTableIndex = SymbolTableIndexMap[R.SymbolName];
+      if (R.SymbolTableIndex)
+        SymbolTableIndex = *R.SymbolTableIndex;
----------------
It should probably be an error somewhere if both fields are set (possibly unless they indicate the same symbol). I'd also write this as a ternary, with the symbol index being the first case, to avoid the lookup if not necessary, unless the intention is to insert a default constructed symbol in the map when no symbol exists?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56294