-
Notifications
You must be signed in to change notification settings - Fork 43
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
Allow multiple Clang
instances per thread.
#46
base: master
Are you sure you want to change the base?
Conversation
13b8812
to
e551c06
Compare
e551c06
to
c511726
Compare
@KyleMayes, I updated this to not use KyleMayes/clang-sys#145. |
522c45a
to
730e04a
Compare
src/lib.rs
Outdated
} | ||
|
||
/// Constructs a new `Clang`. | ||
/// | ||
/// Only one instance of `Clang` is allowed at a time. | ||
/// Only one instance of `Clang` is allowed per thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I'm not actually sure if this true.
Depending on whether libclang
is thread-safe or not, we either need to allow
1 Clang
instance per thread where Clang: !Send + !Sync
or we need to allow
1 Clang
instance per process where Clang: Send
.
7fe5d1d
to
d5aaa64
Compare
@KyleMayes, could you have another look here? |
FWIW #26 was so long ago, I pretty much forgotten all about what it was about. That said, it isn't clear to me some of the premises here are accurate:
Dynamically loading a library is still a process-wide reference-counted affair (this is just how dynamic loaders work). This cannot be changed unless some non-portable |
I used to be somewhat knowledgeable about this kind of thing (loading
libraries and stuff) but I haven't put much thought into this library for
years (except when I need to do essential maintenance to keep it
functioning so that I can fulfill my duty to the Rust ecosystem 🫡).
I'm on vacation for the next week or so, if y'all haven't hashed it out by
then I'll take another look at this.
…On Mon, Nov 14, 2022, 22:12 Simonas Kazlauskas ***@***.***> wrote:
FWIW #26 <#26> was so long
ago, I pretty much forgotten all about what it was about. That said, it
isn't clear to me some of the premises here are correct.
This change allows creating multiple Clang instances per thread while
still only loading libclang once per thread.
Dynamically loading a library is still a process-wide reference-counted
affair (this is just how dynamic loaders work). This cannot be changed
unless some non-portable dl* APIs are used. It just that constructing a
structure containing function pointers to all the symbols is gonna happen
per-thread which seems like a wasted effort, given that the code could just
have a static CLANG: Once<...>.
—
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHYCKEUEU44MSKQA5NKFLLWIKTNRANCNFSM6AAAAAAQOY7ZZY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
That's exactly the problem: Currently, we can only have a single instance per-process, which cannot be shared, so we need one of the two options described in #46 (comment). |
f9ab33b
to
d5aaa64
Compare
Are there any updates on this? I am encountering the same problem. |
Depends on KyleMayes/clang-sys#145.Previously, you could only create one
Clang
across all threads. This is a problem when trying to useClang
in tests which run in parallel. This change allows creating multipleClang
instances per thread while still only loadinglibclang
once per thread.