[PATCH] D26719: Bitcode: Introduce initial multi-module reader API.

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/include/llvm/Bitcode/BitcodeReader.h:48
+    uint64_t IdentificationBit;
+    uint64_t ModuleBit;
+
----------------
Can you document these two fields?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:850
+      ValueList(Context), MetadataList(Context) {
+  this->ProducerIdentification = ProducerIdentification;
+}
----------------
Why not in the initializer list?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:6607
+    ProducerIdentification = *ProducerIdentificationOrErr;
+  }
+
----------------
I think it is used only on error, isn't it?

We may be able to pass the `IdentificationBit` to the BitcodeReader and have it jump there to get the `ProducerIdentification` only if needed.

Might not worth the effort though.


================
Comment at: llvm/test/Bitcode/invalid.test:38
 BAD-TYPE-TABLE-FORWARD-REF: Invalid TYPE table: Only named structs can be forward referenced
-BAD-BITWIDTH: Bitwidth for integer type out of range
+BAD-BITWIDTH: Malformed block
 BAD-ALIGN: Invalid alignment value
----------------
I'm surprised by this change, is it really intended? I suspect that some of your change is losing coverage here.

(Same on most of the changes below)


https://reviews.llvm.org/D26719