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

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1727
+
+  if (Tag % 2)
+    IsIntegerValue = false;
----------------
I don't understand why this line changed, but more importantly, the `2` looks like a magic number and it is unclear why that value is the correct one. Is there another way of writing this that would be more correct (e.g. bitwise `&` against a known flag value)?


================
Comment at: llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test:20
+
+# DISASM:        mul a0, a1, a2
+
----------------
No need for the large amount of whitespace between `DISASM:` and `mul`. One space is sufficient.


================
Comment at: llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test:33
+## The content is the encoding of "mul a0, a1, a2".
+## The encoding could be decoded only when M-extension is enabled.
+    Content: 3385C502
----------------
when the "m" extension


================
Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:8
+//===----------------------------------------------------------------------===//
+#include "llvm/Support/ELFAttributeParser.h"
+#include "llvm/Support/ARMAttributeParser.h"
----------------
I think it's normal to have a blank line after the licence header.


================
Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:17
+static void testParseError(ArrayRef<uint8_t> bytes, const char *msg) {
+  ARMAttributeParser parser;
+  Error e = parser.parse(bytes, support::little);
----------------
If these tests are for the generic parts of the attribute parser, you should probably define a concrete parser type that is neither ARM nor RISC-V, and use that in the tests, e.g. `TestAttributeParser`.

If that's not practical for whatever reason, you need to put a comment somewhere in this file indicating that the `ARMAttributeParser` is used here for generality. Alternatively, consider changing this function to take a template argument for the parser type, and call the test for all instantiated parser types, or simply duplicating the contents of this function, but for the other parser type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023