[PATCH] D33971: Object: Have the irsymtab builder take a string table builder. NFCI.

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 07:25:36 PDT 2017


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

LGTM with the suggested comment and move of the StringSaver declaration.



================
Comment at: llvm/lib/Object/IRSymtab.cpp:52
+  Builder(SmallVector<char, 0> &Symtab, StringTableBuilder &StrtabBuilder,
+          BumpPtrAllocator &Alloc)
+      : Symtab(Symtab), StrtabBuilder(StrtabBuilder), Saver(Alloc) {}
----------------
pcc wrote:
> tejohnson wrote:
> > Why pass in the BumpPtrAllocator as well? I don't see that being shared in this or any follow-on patches?
> The BumpPtrAllocator owns any strings that are created by this code. This is necessary because the StringTableBuilder does not take ownership of any strings that are added to it. This change makes it possible for the lifetime of the strings to extend past the lifetime of the Builder.
Ok, can you document this on the constructor? It would probably be clearer to have a single data structure that owns all these things that need the same lifetime (the StringTableBuilder, BumpPtrAllocator and possibly the StringSaver for simplicity), but that can be considered for a follow on refactoring.


https://reviews.llvm.org/D33971





More information about the llvm-commits mailing list