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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.  Thanks!

I still think you might be able to use an `Optional` (see my inline comment) but I don't need to see this again.



================
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;
+
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47905