[PATCH] D105710: [OpaquePointers][ThreadSanitizer] Cleanup calls to PointerType::getElementType()

Mon Jul 12 14:42:06 PDT 2021

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D105710#2872101 <https://reviews.llvm.org/D105710#2872101>, @aeubanks wrote:

> I still think it's not worth it to find some test that covers this case.
>
> Getting rid of calls to `PointerType::getElementType()` in it of itself is a goal. If we had done this before the opaque pointer type were introduced we wouldn't need tests and it would have gone in as an NFC. As long as we're not adding functionality I don't think tests are super valuable.
>
> IMO we should try to remove as many calls to get pointee types as possible before we start running check-llvm with --force-opaque-pointers and combing through crashes. We'll get the same test coverage either way in the end.

I feel pretty strongly against (on principle - if it's testable, it should be tested - the change isn't NFC now that we have opaque pointer types), but won't hold this up on that.

Might be worth an assert in getMemoryAccessFuncIndex that we've tracked the pointer element type correctly (assert isOpaqueOrPointeeTypeMatches) to demonstrate that the type hasn't changed, we're just tracking it differently.

[Though, OK, I'm a bit hung up on this: Would it be terribly expensive to take an existing test and add a new RUN line to run it with opaque pointers & then it'd be clear that these changes were adequate/necessary/correct to make this code work correctly with opaque pointers]

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105710