[PATCH] D79545: [VE] Implements minimum MC layer for VE (3/4)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Okay, I've had a bit more time to look at the tests. The logic generally looks fine, but there are a number of mostly stylistic issues that need addressing.

clang-format is complaining a lot on some of your new files. Please remember to run it to format them according to the LLVM coding standards.



================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:1
+//===- ELFObjectFileTest.cpp - Tests for Machine field in ELFObjectFile ---===//
+//
----------------
Remove the "Machine field in" bit in the comment. It doesn't add anything, and will be incorrect in the future when we add more tests.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:23-24
+
+// ELF64_DataForTest - a class to initialize a buffer to represent ELF object
+//                     file.
+//
----------------
There is no need to mention the class name in this comment. It doesn't add anything to the comment, because the comment is already associated with the struct by its presence.

Also "to represent an ELF".


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:25-26
+//                     file.
+//
+// FIXME: Initialize not only a buffer but also ELFObjectFile itself.
+struct ELF64_DataForTest {
----------------
Rather than leave this FIXME here, just do it right the first time. You could use something similar to the fallible constructor pattern as described in the [[ https://llvm.org/docs/ProgrammersManual.html#fallible-constructors | LLVM Programmer's Manual ]] to have this object return a an ELFObjectFile instance. You could even consider instead doing the error handling inside the constructor using ASSERT_THAT_EXPECTED to create an ELFObjectFile instance stored in the struct and used by the test.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:27
+// FIXME: Initialize not only a buffer but also ELFObjectFile itself.
+struct ELF64_DataForTest {
+  std::vector<uint8_t> Data;
----------------
The name of the struct says ELF64, but you pass the `Class` into the code, implying it's able to work with ELF32. I don't think you can have both. Better would be to make this class able to work with both ELF32 and ELF64 and generate the data accordingly.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:30
+
+public:
+  ELF64_DataForTest(uint8_t Class, uint8_t Encoding, uint16_t Machine)
----------------
This is a `struct`. No need for `public` here.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:32
+  ELF64_DataForTest(uint8_t Class, uint8_t Encoding, uint16_t Machine)
+      : Data(56) {
+    uint8_t LoMachine = Machine & 0xff;
----------------
I don't think there's any real benefit to pre-sizing this vector, but even if there is, you should use `sizeof(Elf64_Ehdr)` or equivalent for ELF32 rather than hard-coding in the number.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:33-34
+      : Data(56) {
+    uint8_t LoMachine = Machine & 0xff;
+    uint8_t HiMachine = (Machine >> 8) && 0xff;
+    Data.assign({
----------------
You allow passing in of the `Encoding` value, but are hard-coding here an assumption about it being little endian (or more specifically, the same as the host machine). You need to a) handle hosts being either little or big endian (using sys::isLittleEndian) and b) handle different `Encoding` values. There are already functions in LLVM for byte swapping. Consider using those instead, possibly in combination with `reinterpret_cast<uint8_t *>` to convert it to a sequence of bytes.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:60
+
+TEST(ELF64LE_MachineTest, MachineTestForVE) {
+  ELF64_DataForTest Data(ELF::ELFCLASS64, ELF::ELFDATA2LSB, ELF::EM_VE);
----------------
LLVM style doesn't allow names to have underscores in them. I'd also be even more generic with this name and just put something like `ELFObjectFileTest`. This will allow users to just run it specifically alongside other future ELFObjectFile tests.


================
Comment at: llvm/unittests/ObjectYAML/ELFRelocationTypeTest.cpp:1
+//===- ELFRelocationTest.cpp - Tests for relocation type in ELFYAML.cpp ---===//
+//
----------------
Similar to the ELFObjectFile test, this test file should be named after the source file it is testing, and the comment here needs updating.


================
Comment at: llvm/unittests/ObjectYAML/ELFRelocationTypeTest.cpp:36
+  SmallString<0> Storage;
+  auto ExpectedFile = toBinary<ELF64LE>(Storage, R"(
+--- !ELF
----------------
Don't use `auto` where the type isn't obvious. Instead, please use the type explicitly.


================
Comment at: llvm/unittests/ObjectYAML/ELFRelocationTypeTest.cpp:82
+  // Test relocation types.
+  for (const SectionRef &Sec : File.sections()) {
+    Expected<StringRef> NameOrErr = Sec.getName();
----------------
I think `SectionRef` and `RelocationRef` below are intended to be like `StringRef` in that they don't need to be passed around as `const &` since they are lightweight.


================
Comment at: llvm/unittests/ObjectYAML/ELFRelocationTypeTest.cpp:130
+      default:
+        FAIL() << "Find not listed relocation type";
+        break;
----------------
Perhaps "Found unexpected relocation type: " + R.getType() would be a more useful error message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545