[PATCH] D31828: Bitcode: Move version and global value module code parsers to separate functions. NFCI.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:388
+  /// not need this flag.
+  bool UseRelativeIDs = false;
+
----------------
tejohnson wrote:
> pcc wrote:
> > tejohnson wrote:
> > > Why move this into BitcodeReaderBase, rather than BitcodeReader?
> > In D31838 the code that reads names is shared between the module reader and the summary reader. That code needs to know the module version, so I needed to move the module version parsing code into the base class, including the fields that it sets.
> Ok, but if this field is only ever going to be relevant to the BitcodeReader derived class, why not have a virtual method that takes the parsed version and can be used to set up whatever fields are relevant for each derived class? I would imagine this won't be the last time we end up needing to guard behavior specific to only one derived reader based on the version.
Such a method would be called from `{,ModuleSummaryIndex}BitcodeReader:;parseModule` which is already specific to the derived class. If we do what you suggest we might as well inline the parser code into the `parseModule` methods, i.e. what we are already doing. Would you prefer that? I suppose I have no strong objections to it, but figured it may be better to keep track of the version evolution in one place.


https://reviews.llvm.org/D31828