tra added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:25-31
+// Look for sub-directory starts with Prefix under Path. If there is one and
+// only one matching sub-directory found, append the sub-directory to Path. If
+// there is no matching sub-directory or there are more than one matching
+// sub-directories, diagnose them. Returns true if there is only one matching
+// sub-directory.
+static void handleSPACKPackage(const Driver &D, SmallString<0> &Path,
+ StringRef Prefix) {
----------------
`void` function can not return `true`, so the comment needs updating.
Also, Using Path as both an input and an output is odd. We should probably
return the full path or an empty string and call the function
`findSPACKPackage`.
================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:27-28
+// only one matching sub-directory found, append the sub-directory to Path. If
+// there is no matching sub-directory or there are more than one matching
+// sub-directories, diagnose them. Returns true if there is only one matching
+// sub-directory.
----------------
How are users expected to handle cases when they do have multiple rocm versions
installed with spack (I assume that having multiple packages *is* possible.
E.g. package-4.0, package-3.7 should be able to co-exist). They may have
legitimate reasons to have more than one rocm version installed *and* they may
need to be able to build with a particular one.
Can users use `--hip-path=specific/hip/version/in-spack` ?
================
Comment at: clang/lib/Driver/ToolChains/ROCm.h:56
+ // convention <package_name>-<rocm_release_string>-<commit_hash>.
+ llvm::SmallString<0> SPACKReleaseStr;
+ };
----------------
Nit: using `SmallString` here and everywhere else in this patch does not buy us
anything. `std::string` would be fine.
My own rule of thumb is to use standard types by default and optimized types
when they are needed.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97340/new/
https://reviews.llvm.org/D97340
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
