[PATCH] D81585: [AIX][XCOFF][Patch1] Provide decoding trace back table information API for xcoff object file for llvm-objdump -d
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 9 01:15:49 PDT 2020
jhenderson added inline comments.
================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:348
+
+ // Using for the fixed or float point parameter type identification.
+ static constexpr uint32_t FixedParaTypeBit = 0x8000'0000;
----------------
Using -> Used
(?)
================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:408
+ uint64_t Size);
+ XCOFFTracebackTable(const uint8_t *Ptr, uint64_t Size, Error &Err);
+
----------------
Make this private so that all users must use the `create` function.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:883
+XCOFFTracebackTable::XCOFFTracebackTable(const uint8_t *Ptr,
+ const uint64_t Size, Error &Err)
+ : TBPtr(Ptr), Size(Size) {
----------------
Delete the `const` for `Size`.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:886
+ ErrorAsOutParameter EAO(&Err);
+ DataExtractor DE(ArrayRef<uint8_t>(Ptr, Size), /* IsLittleEndian */ false,
+ /* AddressSize */ 0);
----------------
Preferred style is `/* IsLittleEndian=*/false` (same with `AddressSize` below). This plays well with clang-format too.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:888
+ /* AddressSize */ 0);
+ uint64_t OffsetPtr = 0;
+
----------------
This isn't a pointer, so remove the `Ptr`!
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:890
+
+ DE.getU64(&OffsetPtr, &Err);
+
----------------
Not sure if you've come across it yet, but you could simplify all these calls using `DataExtractor::Cursor` instead. This stores the error internally, whilst also allowing you to continue parsing to the end, with no harmful effects (because nothing is read if `Cursor` is in an error state), similar to `Err` being passed in everywhere. It also tracks the offset. Usage is:
```
DataExtractor::Cursor C(/*Offset=*/0);
DE.getU64(C); // By the way, what is this value for?
...
// No need for ifs here.
CodeLen = DE.getU32(C);
HandlerMask = DE.getU32(C);
...
Err = C.takeError();
```
================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:96
+ EXPECT_TRUE(TT1.getParaType());
+ EXPECT_STREQ(TT1.getParaType().getValue().data(), "i, i, f, f, d");
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Does `EXPECT_EQ(TT1.getParaType().getValue(), "i, i, f, f, d");` not work?
>
> according to https://github.com/google/googletest/blob/master/googletest/docs/primer.md
>
> EXPECT_EQ(val1, val2); compare two values. val1 == val2
>
> EXPECT_STREQ(str1,str2); compare two C strings the two C strings have the same content
>
Emphasis on "C strings". You're not using a C string on the left, so can use standard C-string to `std::string` implicit conversion on the right instead of calling `c_str` or `data`, so `EXPECT_EQ` is better (simplfies code).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81585/new/
https://reviews.llvm.org/D81585
More information about the llvm-commits
mailing list