[PATCH] D136344: [ELF][RISCV] Merge `.riscv.attributes` sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Some variable names are overly longer and are unlike the rest of lld/ELF.



================
Comment at: lld/ELF/Arch/RISCV.cpp:824
+
+  SmallVector<InputSectionBase *, 0> attributesSections;
+  // Collect all `.riscv.attributes` sections.
----------------
The name is verbose and isn't like other lld code. Just `sections` is fine since we are in the context of `combineRISCVAttributesSections`


================
Comment at: lld/ELF/Arch/RISCV.cpp:826
+  // Collect all `.riscv.attributes` sections.
+  llvm::copy_if(ctx.inputSections, std::back_inserter(attributesSections),
+                isRISCVAttributesSection);
----------------
This with `erase_if` can be simplified as: `llvm::erase_if(ctx.inputSections, [&](...) { if (!riscvattribute) return false; sections.push_back(...); return true; })` 


================
Comment at: lld/ELF/Arch/RISCV.cpp:828
+                isRISCVAttributesSection);
+  if (!attributesSections.empty()) {
+    InputSectionBase *resultAttributesSection =
----------------
early return is more common


================
Comment at: lld/ELF/Arch/RISCV.cpp:834
+    // Replace all existing attributes sections with merged one.
+    auto attributesSectionPlace =
+        llvm::find_if(ctx.inputSections, isRISCVAttributesSection);
----------------
The name is verbose. Just `place` is fine.


================
Comment at: lld/ELF/Driver.cpp:2818
+    combineRISCVAttributesSections();
+  }
+
----------------
drop braces


================
Comment at: lld/ELF/SyntheticSections.cpp:3691
+  for (const InputSectionBase *s : attributesSections) {
+    RISCVAttributeParser attributesParser;
+    if (Error e = attributesParser.parse(s->data(), support::little))
----------------
`parser`


================
Comment at: lld/ELF/SyntheticSections.cpp:3695
+    for (const auto &currentTag : attributesTags) {
+      unsigned currentAttribute = currentTag.attr;
+      Optional<unsigned> value =
----------------
Just avoid the variable? It isn't much shorter than `currentTag.attr`.

You can use `tag` instead of `currentTag`


================
Comment at: lld/ELF/SyntheticSections.cpp:3721
+    if (!parseResult) {
+      error(std::string("Invalid arch name: ") + archValue.str() + " " +
+            llvm::toString(parseResult.takeError()));
----------------
Prefer lower-case diagnostics. See the coding standard about error messages.


================
Comment at: lld/ELF/SyntheticSections.cpp:3736
+              "Expected rv" +
+              std::to_string(resultXlen) + " configuration.");
+      }
----------------
no trailing dot. avoid `std::to_string`. `error(Twine(some_integer) + "...")`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136344