[PATCH] D139553: [llvm-objdump][RISCV] Use new common method to parse ARCH RISCV attribute

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D139553#4039569 <https://reviews.llvm.org/D139553#4039569>, @kito-cheng wrote:

> ping, @MaskRay could you take a look for this, RISC-V part is LGTM, but this patch has modify something more than RISC-V, so I would like to make sure you are happy with those change too :)

Thanks! This LGTM to me as well.



================
Comment at: llvm/include/llvm/MC/SubtargetFeature.h:192
 
+  void AddFeaturesVector(const std::vector<std::string> &OtherFeatures);
+
----------------
`addFeaturesVector`

Use `ArrayRef` if no need to be a `vector`


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:580
+    ++I;
+    I += ConsumeLength;
+    if (*I == '_')
----------------
`+= 1 + ConsumeLength`


================
Comment at: llvm/test/tools/llvm-objdump/ELF/RISCV/objdump.s:1
+# RUN: llvm-mc -filetype=obj -triple riscv64 < %s | \
+# RUN:     llvm-objdump -d -M no-aliases - | \
----------------
drop `< ` for llvm-mc


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2007
+  if (!FeaturesValue) {
+    WithColor::error(errs(), ToolName) << FeaturesValue.takeError();
+  }
----------------
drop braces


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:567
+  STI.reset(TheTarget->createMCSubtargetInfo(TripleName, "",
+                                             (*Features).getString()));
   if (!STI)
----------------
`(*...).xxx` => `->` 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139553