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

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: include/llvm/Support/DataExtractor.h:54-76
+  class Cursor {
+    uint64_t Offset;
+    Error Err;
+
+    friend class DataExtractor;
+
+  public:
----------------
dblaikie wrote:
> 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.
No, I don't think the user could do much harm. I suppose one could somehow end up accidentally taking the address of the contained Offset member, use that for parsing but then later expect the regular cursor semantics to apply. But that probably not very likely to happen, At the end of the day, it just seems nicer to me to have accessor functions instead of the user twiddling with the fields of the struct directly, but that's highly subjective.


================
Comment at: lib/Support/DataExtractor.cpp:31
+  if (isError(Err))
+    return T(0);
 
----------------
dblaikie wrote:
> You can skip the 0 here, if you like, "return T();" should do the right thing, if I recall correctly.
Yeah, it will, except for the `Uint24` pseudo-type, which does not have the default constructor. I could add the default constructor, but that does not matter now with the new code layout (I think `val = 0` looks better than `val = T()`)


================
Comment at: unittests/Support/DataExtractorTest.cpp:225
+    // succeed.
+    EXPECT_DEATH(C.~Cursor(), "unexpected end of data");
+    EXPECT_THAT_ERROR(C.takeError(), Failed());
----------------
dblaikie wrote:
> 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).
Done. I've gone with a std::unique_ptr because it makes the construction of the cursor slightly nicer (missing an `in_place_t` for llvm::Optional).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713