[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/include/llvm/Support/ELFAttributes.h:32
+                           bool HasTagPrefix = true);
+Optional<int> attrTypeFromString(StringRef Tag, TagNameMap Map);
+
----------------
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.


================
Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:1
+#include "llvm/Support/ELFAttributeParser.h"
+#include "llvm/Support/ARMAttributeParser.h"
----------------
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"


================
Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:5
+#include "gtest/gtest.h"
+#include <string>
+
----------------
I think it's normal to put a blank line between standard library headers and other includes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023