[PATCH] D117870: [OpaquePtrs] Add getNonOpaquePointerElementType() method

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.


================
Comment at: llvm/include/llvm/IR/Type.h:375-378
+  /// Only use this method in code that is not reachable with opaque pointers,
+  /// or part of deprecated methods that will be removed as part of the opaque
+  /// pointers transition.
+  Type *getNonOpaquePointerElementType() const {
----------------
nikic wrote:
> dblaikie wrote:
> > nikic wrote:
> > > dblaikie wrote:
> > > > dexonsmith wrote:
> > > > > Given where this should be used, should all uses be required to be documented with a FIXME/TODO/SOMETHING to indicate which of the two cases it is?
> > > > > 
> > > > > I don't feel strongly (up to you), but it might make it easier to audit them.
> > > > Yeah, or possibly have two different function names to self-document those two cases.
> > > My 2c here is that it doesn't really matter why the method is used, just that the code will get deleted after the migration.
> > > 
> > > I think it's fairly obvious in most cases, because it's either directly in a deprecated API, or shortly after an isOpaque() check. A comment might be warranted if the check is far removed (e.g. the SROA case is a bit non-obvious, because the opaque pointer check happens in a different function.)
> > Ah, it sounded like there are two classes here that will need to be treated differently - like the deprecated uses we would expect to cleanup before the opaque pointer flag flip, but the "not reachable" cases would not be cleaned up before that flip? (or the other way around, or some other variation) Is that not the case? Could you expand a bit on the difference between the two cases described by this comment, and the expected lifecycle/path for them?
> In either case, the calls would have to go away when typed pointer support is dropped. For uses in deprecated methods they *might* go away earlier -- this depends on whether we're going to drop C API support in advance or not. I don't think we have a decision on that yet.
Fair enough - thanks for the explanation!


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

https://reviews.llvm.org/D117870