[PATCH] D81585: [AIX][XCOFF][Patch1] Provide decoding trace back table information API for xcoff object file for llvm-objdump -d

[Digger via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:411
+  static Expected<XCOFFTracebackTable> create(const uint8_t *Ptr,
+                                              uint64_t &Size);
+  uint8_t getVersion() const;
----------------
jasonliu wrote:
> I think a pointer to `Size` is better than reference to `Size` in this case. Reference is too subtle for this scenario and caller might not notice this is taken by reference.
> And as mentioned in the other comment, a comment and a test case is needed for this function.
> 
> For the comment, I would suggest:
> ```
> /// Parse a XCOFF Traceback Table from \a Ptr with \a Size bytes.
> /// Returns a XCOFFTracebackTable upon successful parsing, otherwise an Error is returned.
> ///
> /// \param[in] Ptr
> ///    A pointer points to the beginning of XCOFF Traceback Table (right after the initial 4 bytes of zeros).
> /// \param[in, out] Size
> ///    The length of the XCOFF Traceback Table. 
> ///    If XCOFF Traceback Table is not parsed successfully or there are extra bytes that are not recognized,
> ///    \a Size will be updated to reflect the actual length of the XCOFF Traceback Table that get parsed.
> ```
> 
> Also please include a test that the XCOFFTracebackTable have extra content upon a successful parsing, and check if the Size gets modified correctly to the content that we are able to parse. 
change as suggestion , the test case already there .

EXPECT_EQ(Size, 25u); 

the original Size is 28 bytes. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:857
+      Value <<= 1;
+      continue;
+    } else {
----------------
jasonliu wrote:
> both `continue` (here and in `else`) seems redundant for the current control flow. 
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585