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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3470
+        } else {
+          // If that is not known fall back to the programm adress space.
+          // TODO: can this ever happen? I wasn't able to think of a test case
----------------
Typos programm adress


================
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();
----------------
I prefer to not add code that you can't test and should be unreachable


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3459
+          if (!ExpectedTy->isPointerTy() ||
+              !ExpectedTy->getPointerElementType()->isIntegerTy(8))
+            return error(
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48803