r311065 - Further refactoring of the constant emitter. NFC.

Don Hinton via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 18 12:38:06 PDT 2019


Hi John:

I found this investigating the cast assert noted here:
http://lists.llvm.org/pipermail/cfe-dev/2019-March/061685.html

I subsequently did quick grep and found a number of cases in clang+llvm
(didn't find any in other projects) .  I'm happy to fix these in mass,
i.e., s/cast/dyn_cast/, but as you mentioned, some might require additional
refactoring, e.g., removal of the conditional.

In any case, here's what I've found -- perhaps a new llvm clang-tidy
checker would be useful:

$ find ./clang ./llvm -type f \( -name "*.h" -o -name "*.cpp" \) -exec grep
-Hn "if *(auto.* *= *cast *<" \{\} \;
./clang/lib/Sema/SemaOpenMP.cpp:10904:      else if (auto *DRD =
cast<OMPDeclareReductionDecl>(D))
./clang/lib/CodeGen/CGExprConstant.cpp:1701:  if (auto destPtrTy =
cast<llvm::PointerType>(destTy)) {
./clang/lib/CodeGen/MicrosoftCXXABI.cpp:738:      if (auto *Fn =
cast<llvm::Function>(Throw.getCallee()))
./clang/lib/AST/ASTImporter.cpp:8463:  if (auto *FromDC =
cast<DeclContext>(From)) {
./clang/lib/AST/DeclBase.cpp:1182:    if (auto *Def =
cast<ObjCInterfaceDecl>(this)->getDefinition())
./clang/lib/AST/DeclBase.cpp:1187:    if (auto *Def =
cast<ObjCProtocolDecl>(this)->getDefinition())
./llvm/tools/llvm-objdump/llvm-objdump.cpp:802:  if (auto *Elf64BEObj =
cast<ELF64BEObjectFile>(Obj))
./llvm/tools/llvm-objdump/llvm-objdump.cpp:846:  else if (auto *Elf64BEObj
= cast<ELF64BEObjectFile>(Obj))
./llvm/lib/Target/X86/AsmParser/X86Operand.h:102:        if (auto Imm =
cast<MCConstantExpr>(Val)->getValue())
./llvm/lib/Transforms/IPO/CalledValuePropagation.cpp:139:      } else if
(auto *F = cast<Function>(Key.getPointer()))
./llvm/lib/Transforms/Coroutines/CoroEarly.cpp:183:        if (auto *CII =
cast<CoroIdInst>(&I)) {

thanks...
don

On Mon, Mar 18, 2019 at 12:07 PM John McCall <jmccall at apple.com> wrote:

> On 18 Mar 2019, at 14:39, Don Hinton wrote:
> > It looks like this change introduced a small bug;  Specifically, the
> > following cast test:
> >
> > -      if (auto PT = dyn_cast<llvm::PointerType>(DestTy)) {
> > ...
> > +  // If we're producing a pointer, this is easy.
> > +  if (auto destPtrTy = cast<llvm::PointerType>(destTy)) {
> >
> > Since the cast can fail, shouldn't you prefer dyn_cast<>(), which can
> > return nullptr, over cast<>(), which will assert?
>
> Yes, although if it hasn't caused a problem in the last year and a half,
> maybe we should just change the code to be non-conditional.
>
> John.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190318/b60eeb12/attachment.html>


More information about the cfe-commits mailing list