dylanmckay added a comment.
This will product correct code from an AVR perspective where (before this patch
it would have failed codegen). It is consistent with how we construct function
pointers in LLVM core as well.
I'm not very familiar with Clang internals, one thing that stick out to me:
**The logic instead belongs directly inside the existing
`getTargetAddressSpace` method?**
Perhaps the new `if (is function) { set address space to the value from the
data layout}` logic belongs directly inside `ASTContext::getTargetAddressSpace`
or one of its descendants in the call tree. This would obviously increase the
scope/possible impact of the change further. The reason I suspect it belongs in
there is that in theory, if this is the correct way to get the address space
for a `QualType` (which may be a function) **in this instance**, then I feel
that same logic would hold for any other `QualType` that may be a function
pointer that has `getTargetAddressSpace()` called on it because I don't see
anything special or unique about this existing call to `getTargetAddressSpace`
versus any other existing call to it inside clang. If you implement it at that
lower level inside `getTargetAddressSpace`, your conditional would be something
like `QualType.getTypePtr()->isFunctionType()` etc.
This patch fixes one callsite of `getTargetAddressSpace` but there are several
other existing callsites remaining which if called with a function, they *would
not* return the appropriate address space.
If someone more familar with clang than me disagrees please chime in
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111566/new/
https://reviews.llvm.org/D111566
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
