r311065 - Further refactoring of the constant emitter. NFC.

[John McCall via cfe-commits]

On 18 Mar 2019, at 18:05, John McCall wrote:

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

To get just the Clang ones:

>> ./clang/lib/CodeGen/CGExprConstant.cpp:1701:  if (auto destPtrTy = 
>> cast<llvm::PointerType>(destTy)) {

As discussed, I think this should just be a `cast<>` and
we should drop the dead code following the `if`.

>> ./clang/lib/CodeGen/MicrosoftCXXABI.cpp:738:      if (auto *Fn = 
>> cast<llvm::Function>(Throw.getCallee()))

This should be a `dyn_cast` in case there's an existing declaration.
Well, really `CreateRuntimeFunction` should take a CC as an optional
argument, but that's not something I can ask you to do.

>> ./clang/lib/Sema/SemaOpenMP.cpp:10904:      else if (auto *DRD = 
>> cast<OMPDeclareReductionDecl>(D))

This should be a `dyn_cast`, I think, although again arguably there
should be a more intrusive change to this code that would ensure that
the lookup can only contain these declarations.

>> ./clang/lib/AST/ASTImporter.cpp:8463:  if (auto *FromDC = 
>> cast<DeclContext>(From)) {

It does actually seem to be a general rule that this is only used with
declarations that are DCs, so this can be a `cast<>`, and honestly maybe
it could be updated so that the caller has to pass in a `DeclContext*`.

>> ./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())

I think `getPrimaryContext` is probably only ever used on declarations
that have members, but `dyn_cast` would be the safe thing to do for
both of these.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190318/5fd9cb5b/attachment.html>