Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add methods for LLVMTargetDataRef #141

Merged
merged 8 commits into from
Aug 27, 2020
Merged

Conversation

yowl
Copy link
Contributor

@yowl yowl commented May 30, 2020

This adds methods to LLVMTargetRef for some of the underlying LLVM api calls, covers the ones I want, struct element offsets, and others that I saw that took a LLVMTargetRef as the first parameter. I also added a class that wraps the LLVMTargetRef struct, LLVMTargetData thinking of #140 but not sure I've understood that right as doesn't seem to add much so let me know if that should come out. Added some simple tests for the new methods in LLVMTargetRef to at least make sure they return something sensible.

…methods, better align DataLayout(was LlvmTargetdata) with c++ api method names
@yowl
Copy link
Contributor Author

yowl commented Jun 5, 2020

I've made some changes in line with the feedback, hopefully I'm getting closer. I went for option 2 and created a StructLayout class. It takes both a DataLayout and the StructType as it needs access to the LLVMTargetDataRef to call down. I dropped one of the methods CallFrameAlignmentOfType, as it ends up calling getABITypeAlignment as does ABIAlignmentOfType

 unsigned LLVMABIAlignmentOfType(LLVMTargetDataRef TD, LLVMTypeRef Ty) {
   return unwrap(TD)->getABITypeAlignment(unwrap(Ty));
 }
 
 unsigned LLVMCallFrameAlignmentOfType(LLVMTargetDataRef TD, LLVMTypeRef Ty) {
   return unwrap(TD)->getABITypeAlignment(unwrap(Ty));
 }

So in order to keep the name in line with the c name, I just dropped the duplicate.

@tannergooding
Copy link
Member

Overall this LGTM now, minus the naming nits on methods.

yowl and others added 2 commits August 27, 2020 12:02
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
@yowl
Copy link
Contributor Author

yowl commented Aug 27, 2020

Thanks!

@tannergooding
Copy link
Member

Just wanted to make sure you saw (#141 (comment)):

Likewise, prefix with Get and similar for others below where it is a non-property and the C++ name is get*

I didn't explicitly provide a suggestion for all the names that should be updated and it looks like a couple still remain. https://llvm.org/doxygen/classllvm_1_1DataLayout.html has the C++ names if you need a reference.

@yowl
Copy link
Contributor Author

yowl commented Aug 27, 2020

Thanks, I'll look at the others.

@tannergooding tannergooding merged commit 0e8f58a into dotnet:master Aug 27, 2020
@tannergooding
Copy link
Member

Thanks!

@yowl yowl deleted the targetdata-wrap branch August 28, 2020 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants