[PATCH] D63713: Add error handling to the DataExtractor class

[Pavel Labath via Phabricator via llvm-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: lib/Support/DataExtractor.cpp:17-20
+static Error createError() {
+  return createStringError(errc::illegal_byte_sequence,
+                           "unexpected end of data");
+}
----------------
dblaikie wrote:
> Not sure this function adds value compared to writing the "createStringError" call directly in "unexpectedEndReached"?
Indeed. This was a relict from earlier versions of this patch..


================
Comment at: lib/Support/DataExtractor.cpp:27
+
+static bool isError(Error *E) { return E && *E; }
+
----------------
dblaikie wrote:
> The problem with this is that it clears the "unchecked" bit in the Error, which means code using DataExtractor would not get an assertion failure in Error about it being unchecked.
> 
> Might be worth having a gunit death test to demonstrate that failing to check the Error after parsing some fields does assert/crash.
I already have such tests (DataExtractorDeathTest, below). The reason this works is because `ErrorAsOutParameter` clears the "checked" flag when the function returns.

We could design a way to check this that is statically known to be safe, but I'm not sure if it's worth it for a function that is local to this file (the only method i can think of involves subclassing/wrapping ErrorAsOutParameter).


================
Comment at: lib/Support/DataExtractor.cpp:37
+  uint32_t offset = *offset_ptr;
+  if (LLVM_UNLIKELY(!de->isValidOffsetForDataOfSize(offset, sizeof(T)))) {
+    unexpectedEndReached(Err);
----------------
dblaikie wrote:
> Is the LLVM_UNLIKELY justified by performance data? (again, microbenchmarks could probably justify it in many parts of LLVM where it doesn't make a difference in practice - so I'd be inclined to leave these out for now)
No, I don't have any performance data for this. I'll leave these out...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713