[PATCH] D24595: [lib/LTO] Remove now unneded hack for undefined symbols

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/LTO/LTO.cpp:359
@@ -357,3 +358,3 @@
     // what is the "right" behavior here.
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       auto &CommonRes = RegularLTO.Commons[Sym.getIRName()];
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > Is this ok with  `Sym.getFlags() & object::BasicSymbolRef::SF_Undefined`?
> > The `continue;` you're removing makes us no longer short-circuiting this code.
> I guess we can't have Common and undefined at the same time...
In any case, it will assert now instead of continuing, so we still wouldn't hit this for an undefined (you might be still looking at the older version of the patch).

================
Comment at: lib/LTO/LTO.cpp:390
@@ -388,3 +389,3 @@
   auto ResI = Res.begin();
   for (const InputFile::Symbol &Sym : Input->symbols()) {
     assert(ResI != Res.end());
----------------
pcc wrote:
> davide wrote:
> > mehdi_amini wrote:
> > > We should have the same assert in this loop (maybe in a separate patch)
> > Oh, sure :) I'm still not very familiar with Thin so I didn't think about it. Thanks!
> Or you could add it to `addSymbolToGlobalRes`.
Ah that's a good idea, better to consolidate it there and keep these loops simple.


https://reviews.llvm.org/D24595