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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
