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

Tue Jul 13 10:23:12 PDT 2021

dblaikie added a comment.

In D105710#2872325 <https://reviews.llvm.org/D105710#2872325>, @dblaikie wrote:

> 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]

Any further thoughts on this ^ ? Adding testing in other cases has been possible/practical, such as D105398 <https://reviews.llvm.org/D105398>

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105710