[PATCH] D31838: Bitcode: Add a string table to the bitcode format.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I got through the end of the BitcodeReader.cpp, sending comments so far.



================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:398
+  std::pair<StringRef, ArrayRef<uint64_t>>
+  readNameFromStrtab(ArrayRef<uint64_t> Record);
 
----------------
Document, especially return value.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1799
+    switch (Stream.readRecord(Entry.ID, Record)) {
+    case bitc::VST_CODE_FNENTRY: { // [valueid, offset, namechar x N]
+      // Note that we subtract 1 here because the offset is relative to one word
----------------
We don't have namecharXN anymore, right?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1800
+    case bitc::VST_CODE_FNENTRY: { // [valueid, offset, namechar x N]
+      // Note that we subtract 1 here because the offset is relative to one word
+      // before the start of the identification or module block, which was
----------------
Can this block be refactored out of here and parseValueSymbolTable? I guess that requires some change to recordValue too if we want to share the accessing of the Function pointer, but it would at least be nice to refactor and share the rest.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:2734
+  // v2: [strtab_offset, strtab_size, v1]
+  StringRef Name;
+  std::tie(Name, Record) = readNameFromStrtab(Record);
----------------
Maybe add comment here and in parseFunctionRecord and parseGlobalIndirectSymbolRecord that in v1 the name will be added to the value later when the old style VST is parsed.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4904
+
+          std::string GlobalId =
+              GlobalValue::getGlobalIdentifier(Name, Linkage, SourceFileName);
----------------
refactor out and share with analogous parseValueSymbolTable handling?


https://reviews.llvm.org/D31838