[PATCH] D87697: Do not construct std::string from nullptr

[Yuriy Chernyshov via Phabricator via llvm-commits]

georgthegreat added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1277
     llvm_unreachable("unexpected type");
     break;
   case Type::IntegerTyID: {
----------------
mclow.lists wrote:
> nikic wrote:
> > @mclow.lists There already is a call to llvm_unreachable here ^ so the second call to unreachable seems redundant.
> I know there are compilers that will warn if you get to the end of a fn and there's no return statement there.  Don't know how good clang is about analyzing all the paths through a function and determining that all of them lead to a return (and/or unreachable)
> 
> To make it easy on the compiler (and the reader), if I was writing this, I would move the default case to the end, and make the function look end look like this:
> 
> ```
>     else
>       return "u32";
>   default:
>       break;
>   }
>   llvm_unreachable("unexpected type");
>   return "";
> }
> ```
> 
This patch has been integrated into our monorepo and compiles just find with at least clang7 and clang10.

As all the branches of the switch above result in either return of llvm_unreachable invocation, I have considered the extra llvm_unreachable call redundant.

I do not have any string preferences here. @mclow.lists, what would you suggest after all the considerations?


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1299
   case Type::PointerTyID:
     if (static_cast<const NVPTXTargetMachine &>(TM).is64Bit())
       if (useB4PTR)
----------------
mclow.lists wrote:
> But I would also rewrite this chunk as:
> ```
> if (static_cast<const NVPTXTargetMachine &>(TM).is64Bit())
>     return useB4PTR ? "b64" : "u64";
> else
>     return useB4PTR ? "b32" : "u32";
> ```
> 
> But now I'm indulging in feature-creep :-) so you can ignore this.
This is not the code I am familiar with, so I just wanted to fix the obvious compilation / assertion.


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

https://reviews.llvm.org/D87697