r311065 - Further refactoring of the constant emitter. NFC.

[John McCall via cfe-commits]

On 18 Mar 2019, at 15:38, Don Hinton wrote:
> 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.

They probably all ought to be considered individually.  Tagging Richard 
in case he has opinions about the AST ones.

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

Yeah, it would be great if something like this got run automatically.

John.

> $ 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.
>>