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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/ELFAttributeParser.h:59
+      return None;
+    else
+      return I->second;
----------------
No need for `else` after a return:

```
    if (I == Attributes.end())
      return None;
    return I->second;
```


================
Comment at: llvm/include/llvm/Support/ELFAttributeParser.h:66
+      return None;
+    else
+      return I->second;
----------------
No need for `else` after return.


================
Comment at: llvm/include/llvm/Support/ELFAttributes.h:32
+                           bool HasTagPrefix = true);
+Optional<int> attrTypeFromString(StringRef Tag, TagNameMap Map);
+
----------------
Can this be `Optional<AttrType>`?


================
Comment at: llvm/include/llvm/Support/RISCVAttributes.h:29-34
+  Stack_align = 4,
+  Arch = 5,
+  Unaligned_access = 6,
+  Priv_spec = 8,
+  Priv_spec_minor = 10,
+  Priv_spec_revision = 12,
----------------
HsiangKai wrote:
> jhenderson wrote:
> > These names are not in LLVM style. Are they matching part of the specification? If not, please change them.
> These are some definitions in RISC-V ELF specification. I looked up some ELF definitions in llvm/include/llvm/BinaryFormat/ELF.h. I think it should be fine to include underscore in the names. So, I will capitalize all these names and keep underscores.
Okay, cool. Since these names are part of the standard, they should match exactly what they are in the standard if possible (casing, underscores etc).


================
Comment at: llvm/lib/Support/ARMBuildAttrs.cpp:14
+const TagNameMap llvm::ARMBuildAttrs::ARMAttributeTags = {
+    {ARMBuildAttrs::File, "Tag_File"},
+    {ARMBuildAttrs::Section, "Tag_Section"},
----------------
HsiangKai wrote:
> jhenderson wrote:
> > The formatting changes here seem unrelated so shouldn't be made with this change, right?
> They are formatted by clang-format. Actually, I am a little bit confused whether to apply clang-format on unrelated code or not. I will restore them.
In general, you should only format the code you are working on. Unrelated code should not be changed usually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023