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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: tools/obj2yaml/coff2yaml.cpp:208
       }
-      Rel.SymbolName = *SymbolNameOrErr;
+      if (SymbolUnique[*SymbolNameOrErr])
+        Rel.SymbolName = *SymbolNameOrErr;
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > ruiu wrote:
> > > mstorsjo wrote:
> > > > ruiu wrote:
> > > > > Doesn't this insert a new key if `*SymbolNameOrErr` does not exist in the map? I don't think that affects correctness of the program, but that's probably a waste of time and memory. I'd use `count` instead.
> > > > Yes, but every symbol will exist in the map already. We're not interested in whether the key exists in the map or not (it will always exist), but whether the map entry actually is set to true or false.
> > > Ah OK, then this is fine. I'd still use `count` to not surprise careless readers like me though. :)
> > Why `count`? Wouldn't that just return 1 saying that the symbol indeed is present in the map? We're not interested in whether the key is present or not (it always is), we're interested in the value of the map element, and `count` won't help with that.
> You're imagining a StringSet that only contains the symbols we know are unique. It's not that (building such a data structure would be tricky) but a StringMap<bool> which could be called StringIsUnique, which stores all symbols and whether they are unique or not, as a bool.
Sorry, what I wanted to say is `lookup`.


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

https://reviews.llvm.org/D56294