[PATCH] D127142: [HIP] Link with clang_rt.builtins

[Artem Belevich via Phabricator via cfe-commits]

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