Simplify loading API.

[reitermarkus]

As I understand from KyleMayes/clang-rs#26, the library should only be loaded once per thread.

This changes the load method so it returns an Rc<SharedLibrary>, since you cannot share the library across threads anyways.

This also removes the get/set_library function, which previously claimed they allow sharing the library across threads, which should not be possible.

The unload method returns an error if the current thread still references the library.

[KyleMayes]

Are these changes (and the related ones in clang-sys) driven by a particular need?
Or is it just cleaning up the API and making it harder to misuse (e.g., sharing a libclang instance between threads?
I ask because this would be a breaking change and I don't want to release a v2 of this crate unless I had a really good reason.

[reitermarkus]

The need is for running multiple tests in parallel.

Currently, clang::Clang::new just fails when called a second time.

[reitermarkus]

@KyleMayes, can you have another look here?

[KyleMayes]

I will take another look tomorrow, sorry about the delay

…

On Thu, Oct 20, 2022, 18:31 Markus Reiter ***@***.***> wrote: @KyleMayes <https://github.com/KyleMayes>, can you have another look here? — Reply to this email directly, view it on GitHub <#145 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHYCKESYOGFES3GIUCT5Q3WEHB3PANCNFSM6AAAAAAQOYXFAA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[KyleMayes]

Took another look, I'm very hesitant to merge this because it is a breaking change.

I can see two alternatives:

1. Put this new loading API behind a Cargo feature
2. Just keep the current loading API, which is my preference

I understand that the current loading API is more easily misused, but I don't think it's worth the breaking change.

Your desire to have the ability to have multiple Clang instances in clang-rs should still be possible with the current loading API, correct?

[reitermarkus]

I'm very hesitant to merge this because it is a breaking change.

I don't agree that a breaking change is inherently a reason not to merge this, sometimes breaking stuff in inevitable when improving stuff. That is of course not to say these changes cannot be improved further before releasing this in a new version.

Your desire to have the ability to have multiple Clang instances in clang-rs should still be possible with the current loading API, correct?

I guess it is possible, but this was the cleanest solution I could come up with. I will try to have another look at the clang-rs side of things.

[KyleMayes]

I'm not inherently opposed to breaking changes, it's just that by the nature of this crate and how it is used, non-backwards-compatible versions cause problems in the ecosystem. I know there's stuff like the semver trick but that sounds like it would require significantly more effort than I am willing to put in.

[reitermarkus]

Ah, I see. I didn't think about the types becoming incompatible.