tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:269
+    if (Loc != StringRef::npos)
+      VerStr[Loc] = '.';
+    V.tryParse(VerStr);
----------------
yaxunl wrote:
> tra wrote:
> > This only deals with a single `.` in the version. It's likely that we will 
> > have to deal with a patch release at some point, so you may want to replace 
> > all instances.
> > 
> The ROCm directory follows rocm-{major}.{minor}.{subMinor}[-{build}] format, 
> where major, minor and path (subMinor) are delimited by '.', only the 
> optional build number is delimited by '-'. I have a comment about that at 
> line 266.
Sorry, my mistake. Two of them. I've overlooked that the version had dash in 
it, but we've actually been replacing an underscore.
And then I've missed the commend and assumed that all components are separated 
with the underscore.

The code looks good now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98867/new/

https://reviews.llvm.org/D98867

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to