tra added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:684-690
CmdArgs.append(
{Args.MakeArgString(StringRef("-L") + RocmInstallation.getLibPath()),
"-rpath", Args.MakeArgString(RocmInstallation.getLibPath())});
CmdArgs.push_back("-lamdhip64");
+ CmdArgs.push_back(
+ Args.MakeArgString("-lclang_rt.builtins-" + getTriple().getArchName()));
----------------
MaskRay wrote:
> tra wrote:
> > Nit: Collapse all of these into a single `append()`
> Note: I think multiple push_back isn't that bad...
That's why I've marked it as a "nit".
For arguments, push_back works best for single arguments. append(), when more
than one is needed.
Here, I think an ideal arrangement would look like this:
```
CmdArgs.push_back(Args.MakeArgString(StringRef("-L") +
RocmInstallation.getLibPath();
CmdArgs.append({"-rpath", Args.MakeArgString(RocmInstallation.getLibPath())});
CmdArgs.push_back("-lamdhip64");
CmdArgs.push_back(Args.MakeArgString(getCompilerRT(Args, "builtins")));
```
Single .append({}) works OK for me, too.
Passing a mix of single options and options with arguments via append()
combined with passing multi-word arguments with push_back() looks rather odd in
its inconsistency to me. It makes no practical difference either way, so it's
merely a minor style/readability nit, as far as I'm concerned. I'm fine
deferring to author's preferences.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127142/new/
https://reviews.llvm.org/D127142
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
