tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.
LGTM in general with a minor suggestion.
================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:471
{std::string(SharePath) + "/hip/version",
+ std::string(ParentSharePath) + "/hip/version",
std::string(BinPath) + "/.hipVersion"}) {
----------------
We seem to be rather inconsistent about how we handle paths.
Above, we use `llvm::sys::path::append`, but here we revert to just appending a
path as a string.
I think we should be using llvm::sys::path API consistently. It's unfortunate
that the API does not provide a string-returning function to append elements.
```
auto Append = [](SmallVectorImpl<char> &path, const Twine &a,
const Twine &b = "",
const Twine &c = "",
const Twine &d = "") {
SmallVectorImpl<char> newpath = path;
llvm::sys::path::append(newpath, a,b,c,d);
return newpath;
}
for (const auto &VersionFilePath :
{Append(SharePath, "hip", "version"),
Append(ParentSharePath, "hip", "version"),
Append(BinPath, ".hipVersion")}) {
...
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154077/new/
https://reviews.llvm.org/D154077
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
