[PATCH] D19447: Refactor bitcode writer into classes (NFC)

[Mehdi AMINI via llvm-commits]

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

Look quite straightforward to me (with some inline comments that don't require another round of review)

Makes me think that this file is quite big, should we split it in two and have the summary handling in a separate file?


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:66
@@ -65,2 +65,3 @@
 
-static unsigned GetEncodedCastOpcode(unsigned Opcode) {
+/// Class to manage the bitcode writing for all bitcode file types.
+/// Owns the BitstreamWriter, and includes the main entry point for
----------------
I'd say "Abstract classto manage the bitcode writing, subclassed for each bitcode file type."

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:71
@@ +70,3 @@
+  /// Pointer to the buffer allocated by caller for bitcode writing.
+  SmallVectorImpl<char> *Buffer;
+
----------------
Use a reference is it non-optional and does not need to be rebound.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:100
@@ +99,3 @@
+  uint64_t getVSTOffsetPlaceholder() { return VSTOffsetPlaceholder; }
+  SmallVectorImpl<char> &buffer() { return *Buffer; }
+  BitstreamWriter &stream() { return Stream; }
----------------
May as well provide protected access to the Buffer member directly.
(if you prefer the method, use a verb i.e. `getBuffer()`)



================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:101
@@ +100,3 @@
+  SmallVectorImpl<char> &buffer() { return *Buffer; }
+  BitstreamWriter &stream() { return Stream; }
+  void writeValueSymbolTableForwardDecl();
----------------
Same.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:109
@@ +108,3 @@
+  /// The Module to write to bitcode.
+  const Module *M;
+
----------------
Same as before: reference is so much nicer when applicable :)

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:248
@@ +247,3 @@
+  writeFunction(const Function &F,
+                DenseMap<const Function *, uint64_t> &FunctionToBitcodeIndex);
+  void writeBlockInfo();
----------------
it displays weirdly, but I assume you clang-formatted right?

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:266
@@ +265,3 @@
+  /// The combined index to write to bitcode.
+  const ModuleSummaryIndex *Index;
+
----------------
const ref?

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3373
@@ +3372,3 @@
+
+void IndexBitcodeWriter::writeBlocks() {
+  // Index contains only a single outer (module) block.
----------------
Should we call `writeIdentificationBlock()` here as well? (not as part of this patch, just while I'm looking at it)


http://reviews.llvm.org/D19447