HsiangKai marked 2 inline comments as done.
HsiangKai added inline comments.
================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1727
+
+ if (Tag % 2)
+ IsIntegerValue = false;
----------------
jhenderson wrote:
> 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)?
I have added a comment for it.
================
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);
----------------
jhenderson wrote:
> 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.
Thanks for your suggestions. It makes sense. I have added a concrete parser for
testing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74023/new/
https://reviews.llvm.org/D74023
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
