[PATCH] D103503: [OpaquePtr] Introduce option to force all pointers to be opaque pointers

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/lib/IR/Type.cpp:694-695
 PointerType *PointerType::get(Type *EltTy, unsigned AddressSpace) {
+  if (ForceOpaquePointers)
+    return get(EltTy->getContext(), AddressSpace);
+
----------------
I wonder if this would be better to thread through as a `bool` set on the `LLVMContext` during construction:
```
if (!EltTy->getContext().supportsTypedPointers())
  return get(EltTy->getcontext(), AddressSpace);
```
Might make it easier to reason about / access elsewhere. (Could still have the command-line option control that...)


================
Comment at: llvm/test/Other/force-opaque-ptrs.ll:1
+; RUN: llvm-as --force-opaque-pointers < %s | llvm-dis | FileCheck %s
+
----------------
Can you explain why this test uses `--force-opaque-pointers`? I feel like this test would be more clear if the textual IR actually matched the IR under test (i.e., if it used `ptr`)... also, then the textual IR won't need to be updated later when typed pointers are removed. But maybe there's some limitation you're working around?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103503