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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.


================
Comment at: include/llvm/ObjectYAML/COFFYAML.h:62
   StringRef SymbolName;
+  Optional<uint32_t> SymbolTableIndex;
 };
----------------
alexshap wrote:
> 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 
Sure, I can add a comment.


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

https://reviews.llvm.org/D56294