[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:
> > > 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.
> Actually I was thinking of a pure virtual method that would be called from BitcodeReaderBase::parseVersionRecord and passed the parsed ModuleVersion number. Then each derived reader would define this method as well as the flags specific to that reader. That way this flag could be moved into BitcodeReader, whereas something like the strtab flag being added in your other patch would be defined in BitcodeReaderBase (and set in parseVersionRecord) since it applies to all.
That doesn't really seem like an improvement on the status quo, though (at least IMHO). I'd rather just set everything in the base class, as it's simpler than trying to delegate to the derived classes.

That said I'd be fine with either approach, so let me know if you still want to change it.


https://reviews.llvm.org/D31828