[PATCH] D63713: WIP: DataExtractor error handling

[Pavel Labath via Phabricator via llvm-commits]

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


================
Comment at: lib/Support/DataExtractor.cpp:37
+              bool isLittleEndian, const char *Data, llvm::Error *Err) {
+  setErrorToUnchecked(Err);
   uint32_t offset = *offset_ptr;
----------------
dblaikie wrote:
> I /think/ these should be early-returns if the Error is already set (assuming the goal is to support repeated calls to deserialization functions with the same Error and only check once at the end of the sequence of deserializations)
> 
> Also, you will need to be careful with how you check if the Error is already set - since you want to make sure you don't set the Error to the "checked" state (otherwise the caller won't be forced to check it, which is bad).
> 
> Hmm, perhaps you actually want to test that the Error is checked. If the error is unchecked, then return early & leave it to the caller to check.
> 
> The "unexpectedEndReached" can then just do "if (E) *E = ...;" without having to check the error state.
> 
> If the goal is to treat the Error as a straight out parameter without the repeated deserialization support (because something at a higher level will handle that in some way) - then maybe this should be ErrorAsOutParameter instead?
The goal is "to support repeated calls to deserialization functions with the same Error and only check once at the end of the sequence of deserializations" like you've said.

I also think that it would be nice to have early returns here. The reason I am reluctant to do that is because this will add an additional branch to the hot success path, where we do not encounter an error. In reality, this will probably be two branches because we will also need to check the `Err` pointer, at least in the interim stage.

I hope that this would not matter in practice because once one reaches the end of the deserialization sequence and finds that the error object is set, the only thing he can reliably do is disregard all data he has read since the last time he checked for the error state.

Nonetheless having early returns here would definitely make the API more predictable. I'm going to try to get some numbers on the impact of returning early here...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713