[PATCH] D13666: Add an (optional) identification block in the bitcode

[Jonathan Roelofs via llvm-commits]

jroelofs added inline comments.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3121
@@ +3120,3 @@
+            Major > LLVM_VERSION_MAJOR ||
+            (Major == LLVM_VERSION_MAJOR && Minor > LLVM_VERSION_MINOR)) {
+          auto BitcodeMajor = std::to_string(Major);
----------------
In irc we talked about how to test this... I'll capture those thoughts here:

9:06:40 PM - jokereph_: jroelofs: thks for spotting it
9:08:57 PM - jroelofs: jokereph_: testcases on that particular behavior is probably a good idea (regardless of which epoch kind you guys end up picking)
9:09:31 PM - jokereph_: jroelofs: any idea on how to write it?
9:10:16 PM - jokereph_: I guess I could write a c++ test for that, but it’s not easy to write a generic bitcode test since we want to test something that affects version X.0 only for instance
9:11:19 PM - jroelofs: what if you pull out the "here's what version I am" part of it, and feed that in to the check
9:12:33 PM - jroelofs: then the unitests test could poke in whatever values it wants for that, and have the check verify against poked-in values in the bitcode itself
9:12:41 PM - jokereph_: jroelofs, and writing a C++ unittests? Not a bitcode reader test right? I requires exposing this function though.
9:13:17 PM * jroelofs honestly isn't all that familiar with how the bitcode stuff works/is tested
9:13:30 PM - jokereph_: jroelofs: we only test coarse grain AFAIK
9:13:45 PM - jokereph_: i.e. llvm-dis for instance
9:14:09 PM - jokereph_: I may be able also to write a C++ unittest that forge a bitcode with a specific version and see what the reader says.
9:14:25 PM - jokereph_: In the end it seems quite complicated for what it is :(
9:14:47 PM - jroelofs: maybe split parseBitcodeVersion into a parser and a verifier
9:14:48 PM - jokereph_: I just tested the “major” manually 

10:04:06 PM - jroelofs: jokereph: after thinking about it some more, I think the right thing to do testing-wise is to save off a bitcode file from each time the epoch changes, and then have a lit test that tries to load up each of these files. then check for the expected error message for each file
10:05:32 PM - jroelofs: jokereph: that way if the epoch format gets changed down the line, you've got testcases for all the old formats, and can check that the error code does the right thing for all of them


http://reviews.llvm.org/D13666