search

KyleMayes / clang-sys

Rust bindings for libclang.
Apache License 2.0
128 stars 65 forks source link

Simplify loading API. #145

Closed reitermarkus closed 1 month ago

reitermarkus commented 1 year ago

As I understand from https://github.com/KyleMayes/clang-rs/issues/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 commented 1 year ago

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 commented 1 year ago

The need is for running multiple tests in parallel.

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

reitermarkus commented 1 year ago

@KyleMayes, can you have another look here?

KyleMayes commented 1 year ago

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 https://github.com/KyleMayes/clang-sys/pull/145#issuecomment-1286230286, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHYCKESYOGFES3GIUCT5Q3WEHB3PANCNFSM6AAAAAAQOYXFAA . You are receiving this because you were mentioned.Message ID: @.***>

KyleMayes commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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