[PATCH] D109603: add a new API seek for the Cursor class in the DataExtractor.cpp

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 2 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Support/DataExtractor.h:73
 
+    /// Set the cursor to the new offset.
+    void seek(uint64_t NewOffSet) { Offset = NewOffSet; }
----------------
jhenderson wrote:
> Perhaps add another sentence saying "This does not impact the error state."
thanks


================
Comment at: llvm/unittests/Support/DataExtractorTest.cpp:181
+TEST(DataExtractorTest, Cursor_seek) {
+  DataExtractor DE(StringRef("ABCDEF"), false, 8);
+  DataExtractor::Cursor C(0);
----------------
jhenderson wrote:
> I think you can simplify this testing: `Cursor` doesn't depend on `DataExtractor`, so you can just instantiate a Cursor and then call seek on it a couple of times to show what happens, like the following:
> 
> ```
> Cursor C(5);
> C.seek(3);
> EXPECT_EQ(3u, C.tell());
> C.seek(8);
> EXPECT_EQ(8u, C.tell());
> ```
> 
> This is different to `tell`, where reading data impacts the `tell` location.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109603