[PATCH] D125204: [RISCV] Use RISCVISAInfo to parse arch string from ELF build attribute.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:328-330
+    for (const auto &Feature : FeatureVector) {
+      Features.AddFeature(Feature);
     }
----------------
kito-cheng wrote:
> jhenderson wrote:
> > What does `AddFeature` take? Should Feature be a `StringRef`? If not, I'd still spell out the type rather than use `auto`.
> > 
> > Also, no need for braces for single-line for loops.
> > What does AddFeature take?  Should Feature be a StringRef?
> 
> StringRef, that should take StringRef and take care about the lifetime in my first impression, but I realized that actually store in a std::vector<std::string>[1], so I just pass std::string here.
> 
> [1] https://llvm.org/doxygen/SubtargetFeature_8h_source.html#l00184
> 
Since `AddFeature` takes a `StringRef`, we should just pass one in, i.e. `const std::string &` -> `StringRef`. Lifetime isn't an issue here, so it doesn't matter which we use, but the LLVM guidelines are to use `StringRef`, I believe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125204