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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D19447#409790, @joker.eph wrote:

> 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?


Yes, it does seem large. Unfortunately, the summary handling is relatively quite a bit smaller, so it would still leave a large file. Let me think about what would make sense.


================
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
----------------
joker.eph wrote:
> I'd say "Abstract classto manage the bitcode writing, subclassed for each bitcode file type."
Will update.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:71
@@ +70,3 @@
+  /// Pointer to the buffer allocated by caller for bitcode writing.
+  SmallVectorImpl<char> *Buffer;
+
----------------
joker.eph wrote:
> Use a reference is it non-optional and does not need to be rebound.
> 
In other contexts I have seen the advice that const references should be avoided as class members because of dangers due to lifetime issues if a temporary were to be passed in to the constructor. It looks like this is fairly common practice in LLVM though. I will go ahead and change it since that seems to be the convention here.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:100
@@ +99,3 @@
+  uint64_t getVSTOffsetPlaceholder() { return VSTOffsetPlaceholder; }
+  SmallVectorImpl<char> &buffer() { return *Buffer; }
+  BitstreamWriter &stream() { return Stream; }
----------------
joker.eph wrote:
> May as well provide protected access to the Buffer member directly.
> (if you prefer the method, use a verb i.e. `getBuffer()`)
> 
> 
Yeah, this is what I had done originally, then I switched it to access via a method but it introduces a lot more change (especially for stream()). I will go ahead and change it back.

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

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

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:248
@@ +247,3 @@
+  writeFunction(const Function &F,
+                DenseMap<const Function *, uint64_t> &FunctionToBitcodeIndex);
+  void writeBlockInfo();
----------------
joker.eph wrote:
> it displays weirdly, but I assume you clang-formatted right?
Looks weird to me too. I did clang format, and in fact just tried putting the void writeFunction on the same line and clang-format put it back. It's because the DenseMap is so long.

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

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3373
@@ +3372,3 @@
+
+void IndexBitcodeWriter::writeBlocks() {
+  // Index contains only a single outer (module) block.
----------------
joker.eph wrote:
> Should we call `writeIdentificationBlock()` here as well? (not as part of this patch, just while I'm looking at it)
Yes, I think that would be a good idea. Will add that in a follow-on.


http://reviews.llvm.org/D19447