-
Notifications
You must be signed in to change notification settings - Fork 13
Cratewide API privatisation #318
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
Conversation
CI failed because of clippy. Once UDT is no longer part of public API, clippy starts complaining about its name violating the conventions. |
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.
Why did you decide to not move the commit changing the privacy to the end? This way no commit would emit warnings.
Two functions in `metadata.rs` were missing the `#[no_mangle]` attribute, which is necessary for FFI compatibility: - `cass_materialized_view_meta_partition_key_count` - `cass_materialized_view_meta_clustering_key` This commit adds the attribute to ensure that these functions can be correctly linked from C code.
This serves two purposes: 1. Explicit is better for readability. 2. It avoids a warning about `CASS_COMPRESSION_NONE` being unused once we privatise the module which imports it (in next commits).
UDTDataType had two methods that were not used anywhere in the codebase: - `add_field`; - `get_field_by_index`. These methods were removed not to clutter the codebase with unused code.
The field was not being used anywhere in the codebase, so it's removed.
2bb11ad
to
ac1afb6
Compare
This is a big commit that privatises a lot of items which have been `pub` for no good reason. The goal is to make the public API of the wrapper clear and minimal, and to prevent silencing warnings about unused items (because `pub` items are always considered used). I've kept the `argconv` module public, together with its traits and types, because it's used in doctests. Privatising it would make them stop compiling. Also, those type and traits are actually part of the API, in a sense that they are FFI-facing. So let them stay public.
ac1afb6
to
ea47ecd
Compare
So that checks pass without complaints.
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.
Nice!
Motivation
Lots of items in the driver are exposed as
pub
for no good reason. This involves:pub
structs and enums,pub
traits,pub
fields,pub
functions and methods.This has a number of drawbacks:
cassandra.h
.pub
items are considered used by the compiler.pub
functions must be exposed as proper (noninlined) functions, and they must obey calling conventions.pub
types and theirpub
fields must not be individually optimised, so that other crates can understand how to use them.So privatisation is profitable!
What's done
Privatisation
I perused the whole crate and replaced
pub
withpub(crate)
(or evenpub(self)
) where possible.Found & removed a bug in materialized views API
Two functions in
metadata.rs
were missing the#[no_mangle]
attribute, which is necessary for FFI compatibility:cass_materialized_view_meta_partition_key_count
;cass_materialized_view_meta_clustering_key
.So I added the missing attribute. I have an idea how to prevent similar bugs - will open an issue.
Cleanup after privatisation
A number of items were found to be unused. I either removed them or silenced the warnings.
Pre-review checklist
[ ] I have enabled appropriate tests in.github/workflows/build.yml
ingtest_filter
.[ ] I have enabled appropriate tests in.github/workflows/cassandra.yml
ingtest_filter
.