[PATCH] D127728: [BitcodeReader] Allow reading pointer types from old IR

[Jannik Silvanus via Phabricator via llvm-commits]

jsilvanus accepted this revision.
jsilvanus added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/Bitcode/BitcodeReader.h:73
+struct ParserCallbacks {
+  std::optional<DataLayoutCallbackFuncTy> DataLayout;
+  /// The ValueType callback is called for every function definition or
----------------
jsilvanus wrote:
> Now that the callback itself is optional, and the callback receives the data layout that would be used if not for the callback,
> I'd like to point out that we *could* let the callback return a string instead of an optional string.
> 
> I'd find that slightly better, but I'm not sure it's worth the trouble.
> 
> 
On second thought, returning an optional has the advantage of being explicit about whether the DL str was changed or not, making life easier for the parser. For example, the textual parser keeps track of the source location of the DL string for diagnostics, and discards that location if the callback replaces the DL string. That would be less convenient to do with strings returned.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127728/new/

https://reviews.llvm.org/D127728