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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Bitcode/Writer/ValueEnumerator.cpp:366-389
   // Enumerate the global variables.
-  for (const GlobalVariable &GV : M.globals())
+  for (const GlobalVariable &GV : M.globals()) {
     EnumerateValue(&GV);
+    EnumerateType(GV.getValueType());
+  }
 
   // Enumerate the functions.
----------------
Looks like maybe only one of these code changes is tested - perhaps the test should be expanded to have an example of each of these to exercise each fix here? (maybe in separate patches)


================
Comment at: llvm/test/Other/force-opaque-ptrs.ll:5-7
+define i32* @f(i32* %a) {
+  ret i32* %a
+}
----------------
Would this still exercise the desired codepath if the function was `void()` instead of `ptr(ptr)`? (if I'm understanding correctly, the interesting issue is that the type of `f` itself becomes an opaque pointer - so, oh, you need /some/ interesting type in the function type that would be missed, yeah? What about `void(i32)` would that still reveal the issue/validate the fix?)

I ask because it might be more clear which pointer type becoming opaque is the interesting one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103503