[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:370
+                    : nullptr;
+    }
     return nullptr;
----------------
dim wrote:
> jdoerfert wrote:
> > Consistent style please:
> > 
> > ```
> > if (Value *StrLen = emitStrLen(SrcStr, B, DL, TLI)
> >   return B.CreateGEP(B.getInt8Ty(), SrcStr, StrLen, "strchr");
> > ```
> Consistent with what? :) In this same file, I see at least the following calls to `emitStrLen`, some of which use the `if(!x) return nullptr` spelling, others which use `return x ? y : nullptr`:
> 
> ```
>   Value *DstLen = emitStrLen(Dst, B, DL, TLI);
>   if (!DstLen)
>     return nullptr;
> ```
> 
> ```
>   if (Dst == Src) { // stpcpy(x,x)  -> x+strlen(x)
>     Value *StrLen = emitStrLen(Src, B, DL, TLI);
>     return StrLen ? B.CreateInBoundsGEP(B.getInt8Ty(), Dst, StrLen) : nullptr;
>   }
> ```
> 
> ```
>     Value *StrLen = emitStrLen(CI->getArgOperand(1), B, DL, TLI);
>     if (!StrLen)
>       return nullptr;
> ```
> 
> ```
>     Value *Len = emitStrLen(CI->getArgOperand(2), B, DL, TLI);
>     if (!Len)
>       return nullptr;
> ```
> 
> ```
>     Value *StrLen = emitStrLen(Src, B, DL, TLI);
>     return StrLen ? B.CreateInBoundsGEP(B.getInt8Ty(), Dst, StrLen) : nullptr;
> ```
> 
> But I'm fine with whatever you are suggesting, obviously.  It just seems strange to introduce yet another spelling variant, making it less consistent, not more...
Consistent with the two prior lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70143