[PATCH] D63713: WIP: DataExtractor error handling

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

I like the overall direction of this!



================
Comment at: include/llvm/Support/DataExtractor.h:425
 
+class DataStream {
+public:
----------------
We'll definitely need to add some comment here to motivate its existence, probably contrasting it with the statelessness of the DataExtractor. I saw this patch first and I wondered "why not add this to the DataExtractor directly?", before catching up on the other patch. 


================
Comment at: include/llvm/Support/DataExtractor.h:442
+
+  uint8_t getU8() { return wrap<uint8_t>(&DataExtractor::getU8); }
+  uint8_t *getU8(uint8_t *dst, uint32_t count) {
----------------
labath wrote:
> maybe `readUXX` would be better instead of `getUXX`?
+1


================
Comment at: include/llvm/Support/DataExtractor.h:466
+  uint32_t Offset;
+  bool Error;
+
----------------
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. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713