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

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai marked 2 inline comments as done.
HsiangKai added inline comments.


================
Comment at: llvm/include/llvm/Support/ELFAttributes.h:32
+                           bool HasTagPrefix = true);
+Optional<int> attrTypeFromString(StringRef Tag, TagNameMap Map);
+
----------------
jhenderson wrote:
> HsiangKai wrote:
> > jhenderson wrote:
> > > Can this be `Optional<AttrType>`?
> > `AttrType` is the enum in `ELFAttrs`. There is also `AttrType` in `ARMBuildAttributes` and `RISCVAttributes`. They all could be implicitly converted to integer. So, I use integer as the common interface between different architecture attributes enum.
> Okay, seems reasonable. I wonder if AttrType needs to be specified as having `unsigned` underlying tap (in all cases) to be consistent with the `TagNameItem::attr` member.
OK, I will specify the underlying type explicitly.


================
Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:1
+#include "llvm/Support/ELFAttributeParser.h"
+#include "llvm/Support/ARMAttributeParser.h"
----------------
jhenderson wrote:
> Missing licence header.
> 
> Test files usually are called *Test.cpp, where * is the base file/class that is being tested. It seems like this file is only testing the ARMAttributeParser. Should it be called "ARMAttributeParserTest.cpp"
This file tests the common part of the attribute section, i.e., attribute section header. However, since ELFAttributeParser is an abstract class, I need to use ARMAttributeParser or RISCVAttributeParser as the concrete object to run the test. I just pick ARMAttributeParser as the concrete object. This test is not used to test ARMAttributeParser.

You could refer to https://reviews.llvm.org/D74023#1911405. @MaskRay suggests to extract the common part out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023