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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/Support/DataExtractor.cpp:17-20
+static Error createError() {
+  return createStringError(errc::illegal_byte_sequence,
+                           "unexpected end of data");
+}
----------------
Not sure this function adds value compared to writing the "createStringError" call directly in "unexpectedEndReached"?


================
Comment at: lib/Support/DataExtractor.cpp:27
+
+static bool isError(Error *E) { return E && *E; }
+
----------------
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.


================
Comment at: lib/Support/DataExtractor.cpp:37
+  uint32_t offset = *offset_ptr;
+  if (LLVM_UNLIKELY(!de->isValidOffsetForDataOfSize(offset, sizeof(T)))) {
+    unexpectedEndReached(Err);
----------------
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)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713