[PATCH] D63713: WIP: DataExtractor error handling

Mon Jun 24 09:59:33 PDT 2019

labath marked an inline comment as done.
labath added inline comments.

================
Comment at: include/llvm/Support/DataExtractor.h:466
+  uint32_t Offset;
+  bool Error;
+
----------------
JDevlieghere wrote:
> Should we store an llvm::Error here instead? I guess it doesn't matter much if there's only one error we can detect, but OTOH it'd be nice if we're going to convert them anyway, and all these errors are consistent. 
If we enlist the help of DataExtractor class, then there may be more kinds of errors that we can detect (and I'm thinking we should enlist it, because checking for errors via these offsets is tricky and probably slower than if the DataExtractor set  a flag directly). The one error kind that comes to mind is "invalid uleb128" -- right now the uleb functions will happily read a 1 megabyte uleb, if all the bytes have bit7 set.

Storing an `Error` object as a member variable is a bit tricky due to the checked flag, noncopyability, etc. We could definitely at least have a member function that returns an `Error` object. But I think the first question we need to answer is what kind of error semantics do we want to have here. Should it be something strictly optional (as it is right now), or should it be something that blows up if you forget to check for errors (like `llvm::Error` does)...

I don't really have an opinion here, as I can see a case for both things..

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713