Skip to content
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.

Sign up for GitHub

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

Jump to bottom

build: generate bindings for basic types #188

Merged
merged 15 commits into from
Oct 15, 2024
Merged

build: generate bindings for basic types #188

merged 15 commits into from
Oct 15, 2024

Conversation

muzarski
Copy link
Collaborator

@muzarski muzarski commented Oct 10, 2024 •
edited
Loading

Motivation

All of the numeric types were for some reason defined by us in types.rs. We should let bindgen generate bindings for them.

This is what this PR does. Removes basic types definitions from types.rs and tells bindgen to generate bindings for these types.

size_t

Second commit addresses the issue with size_t binding. There is a question regarding whether we should treat it as usize or not. Please, take a look at that.

Modules refactor

Moved all of the include!s throughout code to one place - lib.rs. Each include! now corresponds to its module.

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.

@muzarski muzarski self-assigned this Oct 10, 2024
@muzarski muzarski mentioned this pull request Oct 10, 2024
Merged
3 tasks
Lorak-mmk
Lorak-mmk approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@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.

I think the current version of the PR is fine (meaning size_t_is_usize should remain false). I see it as a safer option - there is no way for C definition and our definition to diverge.

Note: now the types.rs file doesn't actually define any types. We should probably remove the of the definitions from it.

wprzytula
wprzytula approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

Great to see this fixed.

@Lorak-mmk
Copy link
Collaborator

I think the current version of the PR is fine (meaning size_t_is_usize should remain false). I see it as a safer option - there is no way for C definition and our definition to diverge.

Note: now the types.rs file doesn't actually define any types. We should probably remove the of the definitions from it.

Actually we could, for each generated module, have an entry in lib.rs:

mod generated_mod {
    include!(concat!(env!("OUT_DIR"), "/generated_mod.rs"));
}

That way, lib.rs would be the only file in which we use include! macro - the rest of the project could just use the modules as usual.

@wprzytula
Copy link
Collaborator

That way, lib.rs would be the only file in which we use include! macro - the rest of the project could just use the modules as usual.

Yes, please. I hate those include!s.

@muzarski
Copy link
Collaborator Author

I think the current version of the PR is fine (meaning size_t_is_usize should remain false). I see it as a safer option - there is no way for C definition and our definition to diverge.
Note: now the types.rs file doesn't actually define any types. We should probably remove the of the definitions from it.

Actually we could, for each generated module, have an entry in lib.rs:

mod generated_mod {
    include!(concat!(env!("OUT_DIR"), "/generated_mod.rs"));
}

That way, lib.rs would be the only file in which we use include! macro - the rest of the project could just use the modules as usual.

Done. Adjusted the names of some of the generated .rs files as well.

Lorak-mmk
Lorak-mmk requested changes Oct 11, 2024
View reviewed changes
scylla-rust-wrapper/src/lib.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/lib.rs Outdated Show resolved Hide resolved
@muzarski
Copy link
Collaborator Author

v2: Created cass_consistency_types and cass_batch_types module (instead of having common cass_misc_types module). Moved CassWriteType to cass_error_types module.

@muzarski muzarski requested a review from Lorak-mmk October 14, 2024 11:41
wprzytula
wprzytula approved these changes Oct 15, 2024
View reviewed changes
scylla-rust-wrapper/src/lib.rs Outdated Show resolved Hide resolved
@muzarski
build: generate bindings for basic types
a425c4b 
All of the numeric types were for some reason defined by us
in `types.rs`. We should let `bindgen` define them.

This is what this commit does. Removes basic types definitions
from `types.rs` and tells bindgen to generate bindings for these
types.

Note: size_t type is a bit special, and will be addressed in another commit.
@muzarski
build: generate definition for size_t
a26efb7 
In this commit we tell bindgen to generate a binding for size_t type.
Actually, we only tell it not to treat `size_t` as `usize` (which is
done by default).

Why there were no clashes with `size_t` definition, even though
we used `.allowlist("size_t")`?
The docstring of `.size_t_is_usize()` states:
```
Set whether size_t should be translated to usize.

If size_t is translated to usize, type definitions for size_t will not be emitted.

size_t is translated to usize by default.
```

This means that previously (by default), definition for `size_t` was not
generated in `basic_types.rs`. What happens now is the definition is
generated based on the architecture.
In my case it's std::os::raw::c_ulong (equivalent to u64).

I'm not really sure whether we should treat `size_t` as `usize`
(i.e. create an alias `type size_t = usize`) or not (i.e. `.size_t_is_usize(false)`).
This is my question to reviewers :).
@muzarski
mod: create date_time module
3e31758 
Moved all of the conversions related to date and time
to a separate module - date_time.rs
@muzarski
Copy link
Collaborator Author

Rebased on main. I'll apply @wprzytula 's suggestion in the next push

@muzarski
mod: introduce a handful macro for including generated files
6fc4b36 
This is going to be used in following commits - we are planning
to create a separate module per generated file. No need to
repeat the concatenation logic each time.
@muzarski
mod: move types module to lib.rs
7ef3c3e
@muzarski
mod: introduce cass_error_types module
b1b490b 
Moved the include!(...) that defines `CassError`
and `CassErrorSource` to separate module in lib.rs.

Types definition are re-exported in `cass_error.rs` as well.
@muzarski
mod: introduced cass_value_type module
fb2093a
@muzarski
mod: introduce cass_consistency_types module
c54d09a
@muzarski muzarski requested a review from wprzytula October 15, 2024 15:55
@muzarski muzarski merged commit 3bb9a49 into scylladb:master Oct 15, 2024
11 checks passed
@muzarski muzarski deleted the fix-bindgen-sizet branch October 15, 2024 16:23
@muzarski muzarski mentioned this pull request Nov 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wprzytula wprzytula wprzytula approved these changes

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

Assignees

@muzarski muzarski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@muzarski @Lorak-mmk @wprzytula