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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/ObjectYAML/COFFYAML.h:62
   StringRef SymbolName;
+  Optional<uint32_t> SymbolTableIndex;
 };
----------------
mstorsjo wrote:
> jhenderson wrote:
> > SymbolTableIndex -> SymbolIndex
> > 
> > "SymbolTableIndex" to me implies the section index of the symbol table.
> Well the field in the actual relocation also is called SymbolTableIndex, so I chose that as it's an exact match of what's stored on disk. In coff, the symbol table isn't stored in a section, so there shouldn't be any confusion with that (in a pure coff environment).
Okay, fair enough. Keep it as is.


================
Comment at: tools/obj2yaml/coff2yaml.cpp:146
 
+  StringMap<size_t> SymbolOccurrances;
+  for (const auto &S : Obj.symbols()) {
----------------
mstorsjo wrote:
> jhenderson wrote:
> > SymbolOccurrances -> SymbolOccurrences
> Ok, will fix.
> 
> Later I though of using a StringMap<bool> SymbolUnique instead, to conserve space a little.
Yes, that would make sense.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56294