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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/DataExtractor.h:54-76
+  class Cursor {
+    uint64_t Offset;
+    Error Err;
+
+    friend class DataExtractor;
+
+  public:
----------------
I don't feel strongly, but figured I'd mention it - do you reckon this encapsulation is worthwhile compared to having a struct with the Offset and Error being public members? It doesn't look like there'd be a lot of misuse if the two members were public.


================
Comment at: lib/Support/DataExtractor.cpp:31
+  if (isError(Err))
+    return T(0);
 
----------------
You can skip the 0 here, if you like, "return T();" should do the right thing, if I recall correctly.


================
Comment at: lib/Support/DataExtractor.cpp:36
+    unexpectedEndReached(Err);
+    return T(0);
   }
----------------
And here


================
Comment at: lib/Support/DataExtractor.cpp:38
   }
+  T val = 0;
+  std::memcpy(&val, &Data[offset], sizeof(val));
----------------
But maybe it'd be simpler to move "T val = 0;" (or "T val = T();") earlier and "return val" in those places that have "return T(0);"?


================
Comment at: unittests/Support/DataExtractorTest.cpp:225
+    // succeed.
+    EXPECT_DEATH(C.~Cursor(), "unexpected end of data");
+    EXPECT_THAT_ERROR(C.takeError(), Failed());
----------------
This produces undefined behavior in the non-death case (the dtor will run twice - importantly it'll run on something that isn't an object of the right type (because that object's already been destroyed)

You could use an Optional<Cursor> to have control over the point of destruction (then "EXPECT_DEATH(opt.reset()..." to observe the unchecked Error).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713