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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: include/llvm/ObjectYAML/COFFYAML.h:62
   StringRef SymbolName;
+  Optional<uint32_t> SymbolTableIndex;
 };
----------------
jhenderson wrote:
> 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.
I'd probably add a comment somewhere explaining that this "duplication" (the presence of both Index and Name) are for convenience (that  i.e. in YAML one can specify either the name or the index) + handling of non-unique names. P.S. if i understood the rationale for this change correctly, of course 


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

https://reviews.llvm.org/D56294