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