[PATCH] D48803: Place the BlockAddress type in the address space of the containing function

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added a comment.

Thanks for the review, will update shortly.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3471-3473
+          // TODO: can this ever happen? I wasn't able to think of a test case
+          //  where this would be triggered.
+          FwdDeclAS = M->getDataLayout().getProgramAddressSpace();
----------------
arsenm wrote:
> I prefer to not add code that you can't test and should be unreachable
Sounds good, will change to llvm_unreachable("").


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3459
+          if (!ExpectedTy->isPointerTy() ||
+              !ExpectedTy->getPointerElementType()->isIntegerTy(8))
+            return error(
----------------
arsenm wrote:
> arichardson wrote:
> > arsenm wrote:
> > > This should not depend on pointee element types
> > I've kept this check for now (with a TODO) and only updated the error message.
> No, just rip out the type check. It's not needed now, and if it is, it's the verifier's responsibility to check it
Will move check to verifier if there isn't one already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48803