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

Peter Collingbourne via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 20 21:54:24 PDT 2017


pcc added inline comments.


================
Comment at: llvm/lib/Object/IRSymtab.cpp:52
+  Builder(SmallVector<char, 0> &Symtab, StringTableBuilder &StrtabBuilder,
+          BumpPtrAllocator &Alloc)
+      : Symtab(Symtab), StrtabBuilder(StrtabBuilder), Saver(Alloc) {}
----------------
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.


================
Comment at: llvm/lib/Object/IRSymtab.cpp:55
 
-  StringTableBuilder StrtabBuilder{StringTableBuilder::RAW};
-
-  BumpPtrAllocator Alloc;
-  StringSaver Saver{Alloc};
+  StringSaver Saver;
 
----------------
tejohnson wrote:
> Move this data member up above constructor that now sets it, adjacent to the other members initialized by the constructor.
Will do


================
Comment at: llvm/lib/Object/IRSymtab.cpp:241
   COFFLinkerOptsOS.flush();
-  setStr(Hdr.COFFLinkerOpts, COFFLinkerOpts);
+  setStr(Hdr.COFFLinkerOpts, Saver.save(COFFLinkerOpts));
 
----------------
tejohnson wrote:
> Why this change? We had a StringSaver before, and weren't using it here.
Previously the lifetime of StrtabBuilder was effectively [1] shorter than that of the COFFLinkerOpts string, so StrtabBuilder was able to share ownership of the string with COFFLinkerOpts. Now that StrtabBuilder outlives the string, we need to save it in the StringSaver.

[1] "Effectively" meaning that we were done with StrtabBuilder before returning from build(). Of course StrtabBuilder's actual lifetime was longer because it is a field. I guess we were just getting lucky that its destructor presumably didn't need to access the string.


https://reviews.llvm.org/D33971





More information about the llvm-commits mailing list