[PATCH] D32070: Object: Shrink the size of irsymtab::Symbol by a word. NFCI.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Object/IRSymtab.h:288
+  return {SymbolRef(Symbols.begin(), Symbols.end(), Uncommons.begin(), this),
+          SymbolRef(Symbols.end(), Symbols.end(), nullptr, this)};
 }
----------------
pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > Should this be Uncommons.end() instead of nullptr? Otherwise, will UncI ever be nullptr allowing the SymbolRef::operator== to detect the end of the symbol_range? I see where we increment UncI, but not how it could be nullptr at the end of the Uncommons range.
> > > We only use `SymI` for iterator comparisons, so it doesn't matter what value `UncI` has at the end of the range.
> > Ok, missed that. Still, it seems odd not just to use Uncommons.end() here, instead of the less-meaningful nullptr. Suggest changing if that doesn't hurt anything.
> I think that would be a little confusing. If I were reading this code and I saw `Uncommons.end()` here I would draw the conclusion that the pointer value in the end iterator is probably being used somehow, which would lead me to believe that there may be a bug in `module_symbols()` because it is passing `nullptr`. Using `nullptr` in both places makes it clear that the value is unimportant.
I guess it isn't straightforward to get the end iterator in the module_symbols case. Fair enough.


https://reviews.llvm.org/D32070