[PATCH] D47905: [ThinLTO] Parse module summary index from assembly

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:775-776
+  // don't have the string owned elsewhere (e.g. the Strtab on a module).
+  StringSaver Saver;
+  BumpPtrAllocator Alloc;
+
----------------
dexonsmith wrote:
> tejohnson wrote:
> > dexonsmith wrote:
> > > The comment suggests these aren't always used.  Should they be in an optional?
> > Looks like I forgot to create the patch as a diff based on a couple other patches I pulled out of this big one. I'll see if I can fix the patch to rebase on top of those. Note this code in particular I pulled out into a separate patch D47842.
> > 
> > Regarding your suggestion, I'm not sure we can use Optional here since StringSaver is non-copyable.
> > 
> > (I also pulled out some other functionality into D47844).
> > I'm not sure we can use Optional here since StringSaver is non-copyable.
> 
> `Optional` doesn't require copyable or moveable.  You can construct in place using the `Optional::emplace` member.
Thanks, I missed that. The emplace solves another issue for me with the string saver change as well. I'm fixing this on D47842 (the patch with just the string saver change), and will add you to the review there. I'm going to add you to D47844 as well (the other patch this one depends on).


Repository:
  rL LLVM

https://reviews.llvm.org/D47905