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

[Dimitry Andric via Phabricator via cfe-commits]

dim marked an inline comment as done.
dim added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:370
+                    : nullptr;
+    }
     return nullptr;
----------------
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70143