[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The code generally looks good. For unittests, I think we can either make llvm-readobj -A canonical or the unittests canonical. If we decide to place tests on one place, we should delete most tests on the other side.

My current preference is that we use more of unittests and leave the minimum to `test/llvm-readobj/ELF/{ARM,RISCV}/`



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1689
+/// parseDirectiveAttribute
+///  ::= .attribute int, int [, "str"]
+///  ::= .attribute Tag_name, int [, "str"]
----------------
No space before `[`


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1723
+
+  StringRef StringValue = "";
+  int64_t IntegerValue = 0;
----------------
Delete `= ""`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023