[PATCH] D125204: [RISCV] Use RISCVISAInfo to parse arch string from ELF build attribute.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 11 23:44:03 PDT 2022
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
More information about the llvm-commits
mailing list