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

Wed May 13 01:34:54 PDT 2020

jhenderson added a subscriber: grimar.
jhenderson added a comment.

Having been looking at another piece of work in the Object library, rather than introducing lit tests for it to cover the specific behaviour, I think gtest unit tests should be introduced instead to cover the behaviour. For more thoughts on this, see D79612 <https://reviews.llvm.org/D79612>.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/machine-enum.test:378-379
+
+# FIXME: EM_ECOG1 and EM_ECOG1X shares ID 168, so one defined latter, EMECOG1X,
+#        is not able to test.
+# FIXME: yaml2obj %s -o %t.ecog1x.o -D MACHINE=EM_ECOG1X
----------------
I'd move this FIXME comment into the code, highlighting that it is dead code, due to the shared value, and then delete this case here, as it looks like it's meant to be a test, but isn't, which might confuse people in the future.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545