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

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 marked 11 inline comments as done.
kaz7 added a comment.

OK, I will run clang-format.  It modifies ENUM_ENT definitions in ELFDumper.cpp, so that I was not running it in this patch since I feel modifying existing format is not good.  Maybe I should think like this patch is a good practice to update existing format.  Thanks.



================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:25-26
+//                     file.
+//
+// FIXME: Initialize not only a buffer but also ELFObjectFile itself.
+struct ELF64_DataForTest {
----------------
jhenderson wrote:
> 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.
Thanks for the link.  I couldn't handle it correctly at first try.  I would like to try it if I have time.

On the other hand, we don't need to hold ELFObjectFile in this structure since I'm not using test fixtures yet.  I supported not only EM_VE but also X86_64, 385, all ELF64, ELF32, big endian, little endian MIPS cases in this test.  I appreciate if I can stop extending and generalizing this test case here at least for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545