Skip to content

Conversation

wprzytula
Copy link
Contributor

@wprzytula wprzytula commented Jun 6, 2025

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:

  1. [Lack of clarity] It's unclear what items are part of the true public API. It's, fortunately, a less of an issue in the specific case of CPP-Rust Driver, because the whole API is defined in cassandra.h.
  2. [Clutter] Items that are unused are not detected by static checks. This is because pub items are considered used by the compiler.
  3. [Small performance penalty] pub functions must be exposed as proper (noninlined) functions, and they must obey calling conventions. pub types and their pub 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 with pub(crate) (or even pub(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 split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have implemented Rust unit tests for the features/changes introduced.
  • [ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • [ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

@wprzytula wprzytula requested a review from Lorak-mmk June 6, 2025 18:25
@wprzytula wprzytula self-assigned this Jun 6, 2025
@wprzytula wprzytula added this to the 0.5 milestone Jun 6, 2025
@wprzytula wprzytula added the bug Something isn't working label Jun 6, 2025
@wprzytula
Copy link
Contributor Author

CI failed because of clippy. Once UDT is no longer part of public API, clippy starts complaining about its name violating the conventions.
As a result, we should merge #317 first.

Copy link
Contributor

@Lorak-mmk Lorak-mmk left a 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.
@wprzytula wprzytula force-pushed the cratewide-privatisation branch from 2bb11ad to ac1afb6 Compare June 11, 2025 14:01
@wprzytula wprzytula requested a review from Lorak-mmk June 11, 2025 14:01
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.
@wprzytula wprzytula force-pushed the cratewide-privatisation branch from ac1afb6 to ea47ecd Compare June 11, 2025 17:06
So that checks pass without complaints.
Copy link
Contributor

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@wprzytula wprzytula merged commit db766eb into scylladb:master Jun 11, 2025
18 of 19 checks passed
@wprzytula wprzytula deleted the cratewide-privatisation branch June 11, 2025 20:08
@wprzytula wprzytula mentioned this pull request Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants