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

Wrong thread-safety trait implementing on LLVMString #488

Open
GCPanic opened this issue Apr 15, 2024 · 3 comments
Open

Wrong thread-safety trait implementing on LLVMString #488

GCPanic opened this issue Apr 15, 2024 · 3 comments
Milestone

Comments

@GCPanic
Copy link

GCPanic commented Apr 15, 2024

Describe the Bug
Currently, inkwell::support::LLVMString implementing !Send, !Sync and Deref<Target = CStr>. This is clearly incorrect, as CStr implementing Send and Sync.

To Reproduce
Unrelated

Expected Behavior
LLVMString is just an CStr with custom Drop, so it should be safe to send it's reference to other threads.

On the other hands, i've searched some informations about LLVM Messages, and i don't found any documents saying it's not thread-safe. In this case, LLVMString should implement Send as well.

LLVM Version (please complete the following information):

  • LLVM Version: [Unrelated]
  • Inkwell Branch Used: [Unrelated]

Desktop (please complete the following information):

  • OS: [Unrelated]

Additional Context
None

@TheDan64
Copy link
Owner

TheDan64 commented Apr 15, 2024

I agree that I think LLVMString should be Send (and maybe Sync?), but I think your analysis is slightly off in that it's more like CString, not CStr. &CStr (which is like &str) shouldn't be Send nor Sync when the reference is non static.

@TheDan64 TheDan64 added this to the 0.5.0 milestone Apr 15, 2024
@GCPanic
Copy link
Author

GCPanic commented Apr 18, 2024

it's more like CString, not CStr.

Yes, you're right. LLVMString has string's ownership, it more like a CString.

&CStr (which is like &str) shouldn't be Send nor Sync when the reference is non static.

Not exactly, although thread::spawn does ask for values to be 'static, Send and Sync trait has nothing to do with lifetimes. And, infact, CStr does implements Send and Sync(which means &CStr also implements them)

@TheDan64
Copy link
Owner

Ah yeah, good point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants