[PATCH] D156928: [Clang][AMDGPU] Fix handling of -mcode-object-version=none arg

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/include/clang/Basic/TargetOptions.h:90
+    COV_Default = 400,
+    COV_MAX = 500
   };
----------------
Typically we just put a `COV_LAST` to indicate that it's over the accepted enumerations.


================
Comment at: clang/lib/Driver/ToolChain.cpp:1364
     // at all, target and host share a toolchain.
     if (A->getOption().matches(options::OPT_m_Group)) {
       if (SameTripleAsHost)
----------------
Is this flag not in the `m` group? It should be caught here right?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1058
     unsigned CodeObjVer = getAMDGPUCodeObjectVersion(D, Args);
+    if(CodeObjVer != 0) {
     CmdArgs.insert(CmdArgs.begin() + 1,
----------------
Use clang-format.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1066
+    if (!IsCC1As) {
+      std::string CodeObjVerStr = (CodeObjVer ? Twine(CodeObjVer) : "none").str();
       CmdArgs.insert(CmdArgs.begin() + 1,
----------------
arsenm wrote:
> don't need to go through std::string? stick with Twine everywhere?
You shouldn't assign to a Twine, but in general I think we should probably put this ternary in-line with the other stuff to avoid the temporary.

The handling here is a little confusing, we do
```
Args.getLastArg(options::OPT_mcode_object_version_EQ);
```
Which expects a number, if it's not present we get an empty string which default converts to zero which we then convert into "none"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156928