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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Minor stuff.



================
Comment at: include/llvm/Support/DataExtractor.h:146
   ///     failure.
-  uint64_t getUnsigned(uint32_t *offset_ptr, uint32_t byte_size) const;
+  uint64_t getUnsigned(uint32_t *offset_ptr, uint32_t byte_size,
+                       Error *Err = nullptr) const;
----------------
Need to add Err to the doxygen.


================
Comment at: include/llvm/Support/DataExtractor.h:213
   ///     The extracted uint8_t value.
-  uint8_t getU8(uint32_t *offset_ptr) const;
+  uint8_t getU8(uint32_t *offset_ptr, Error *Err = nullptr) const;
+  uint8_t getU8(Cursor &C) const { return getU8(&C.Offset, &C.Err); }
----------------
Need to add Err to the doxygen.


================
Comment at: include/llvm/Support/DataExtractor.h:250
+    // This relies on the fact that getU8 will not attempt to write to the
+    // buffer it isValidOffsetForDataOfSize(C.Offset, Count) is false.
+    getU8(C, reinterpret_cast<uint8_t *>(Dst.data()), Count);
----------------
it -> if


================
Comment at: include/llvm/Support/DataExtractor.h:270
   //------------------------------------------------------------------
-  uint16_t getU16(uint32_t *offset_ptr) const;
+  uint16_t getU16(uint32_t *offset_ptr, Error *Err = nullptr) const;
+  uint16_t getU16(Cursor &C) const { return getU16(&C.Offset, &C.Err); }
----------------
Need to add Err to the doxygen.


================
Comment at: include/llvm/Support/DataExtractor.h:329
   ///     The extracted uint32_t value.
-  uint32_t getU32(uint32_t *offset_ptr) const;
+  uint32_t getU32(uint32_t *offset_ptr, Error *Err = nullptr) const;
+  uint32_t getU32(Cursor &C) const { return getU32(&C.Offset, &C.Err); }
----------------
Need to add Err to the doxygen.


================
Comment at: include/llvm/Support/DataExtractor.h:371
   ///     The extracted uint64_t value.
-  uint64_t getU64(uint32_t *offset_ptr) const;
+  uint64_t getU64(uint32_t *offset_ptr, Error *Err = nullptr) const;
+  uint64_t getU64(Cursor &C) const { return getU64(&C.Offset, &C.Err); }
----------------
Need to add Err to the doxygen.


================
Comment at: include/llvm/Support/DataExtractor.h:433
   ///     The extracted unsigned integer value.
-  uint64_t getULEB128(uint32_t *offset_ptr) const;
+  uint64_t getULEB128(uint32_t *offset_ptr, llvm::Error *Err = nullptr) const;
+  uint64_t getULEB128(Cursor &C) const { return getULEB128(&C.Offset, &C.Err); }
----------------
Need to add Err to the doxygen.


================
Comment at: include/llvm/Support/DataExtractor.h:437
+  void skip(Cursor &C, uint32_t Length) const;
+  bool eof(const Cursor &C) const { return Data.size() == C.Offset; }
 
----------------
So eof() intentionally ignores the error state?  (Just making sure the contract is understood.)


================
Comment at: unittests/Support/DataExtractorTest.cpp:10
 #include "llvm/Support/DataExtractor.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
----------------
The Error is part of the DataExtractor interface, you should not need to #include this here.


================
Comment at: unittests/Support/DataExtractorTest.cpp:266
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+}
+
----------------
Test for eof() in the error case? (attempt to read too far, but eof is still false, if that's the correct contract.)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63713