[PATCH] Refactor bitcode reader to simplify control.

[Jan Voung]

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:243
@@ -221,1 +242,3 @@
+  /// The number of modules read at the top level.
+  bool NumModulesParsed = 0;
 
----------------
nit: This is usually 0 or 1, but it seems unexpected for this field to be named "NumModulesParsed", and yet have the type be "bool". Rename or change type?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3113
@@ +3112,3 @@
+    if (std::error_code EC = MaterializeModule(TheModule))
+      return EC;
+  } else {
----------------
Does this need to be cleanupOnError(EC) also?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3151
@@ +3150,3 @@
+    return Error("Can't continue, bitcode error already found");
+  default:
+    // Restore input position to saved position on last call.
----------------
This covers two states? InsideModule and AtTopLevel? It might be more clear if you list them out so it's clear what the "break" corresponds to (AtTopLevel).

Previously, the Stream.JumpToBit(NextUnreadBit); was only needed when InsideModule... is it now needed for AtTopLevel too?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3244
@@ +3243,3 @@
+  default:
+    llvm_unreachable("finishParse exits with ParseState < NoMorInput");
+  }
----------------
"NoMorInput" -> "NoMoreInput"

It could also be that more states >= NoMoreInput are added as code evolves, but not handled here. Can the compiler accept/handle a "static_assert(ParseState < NoMoreInput, "...") to catch what happens if more states are added after NoMoreInput but not handled by this switch?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4518
@@ +4517,3 @@
+
+  if (std::error_code EC = materializeForwardReferencedFunctions())
+    return EC;
----------------
Is this necessary at this point? Should that already be covered by the " // Iterate over the module, deserializing any functions that are still on disk" loop?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4522
@@ -4387,3 +4521,3 @@
   // Check that all block address forward references got resolved (as we
   // promised above).
   if (!BasicBlockFwdRefs.empty())
----------------
The "promise" comment from "above" is removed now, so you could update this comment.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:424
@@ +423,3 @@
+
+  std::error_code updateParseState() {
+    NextUnreadBit = Stream.GetCurrentBitNo();
----------------
kschimpf wrote:
> jvoung wrote:
> > This is a bit weird to me... you have a bunch of function called "updateParseState" but some variants modify ParseState and some variants don't.
> The refactoring is a bit better now. Hopefully good enough.
Thanks -- this is better.

For a while I was also wondering how many places need to be aware of setting the state to ParseError, but I think it's just continueParse() because most/all searching for bit position, etc. goes through that.

http://reviews.llvm.org/D8786

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/