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

Hsiangkai Wang via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Mar 18 19:32:31 PDT 2020


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





More information about the lldb-commits mailing list