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