[PATCH] D74466: [mlir] StdToLLVM: Add error when the sourceMemRef of a subview is not a llvm type.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:389
+  structType = value.getType().dyn_cast<LLVM::LLVMType>();
+  assert(structType && "expected llvm type");
 }
----------------
ftynse wrote:
> mehdi_amini wrote:
> > Something does not line up here: you are using dyn_cast which is supposed to be expecting a possible nullptr as a result, but then you add adding an assert, which implies this is an impossible case...
> > 
> > If you are using the assert just to get a “nicer” crash, then the pattern should be asserting on “isa<>” and using cast<>
> It is indeed for a more explicative assert message.
> 
> What you say sort of contradicts http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates which says "you should not use an isa<> test followed by a cast<>".
You're right: but that is because the correct way is to use cast! 

I'm only mentioning the the isa<> is only because you want a nicer error message with the assert, otherwise cast<> is idiom to follow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74466