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.

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

refactor(foreign): move implementations to dedicated, structured directory #189

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

sivizius
Copy link
Contributor

At the moment, the trait-definition and all implementations for foreign types are located in lib.rs with 1732 LOC. IMHO this is difficult to maintain, especially when adding additional implementations. By moving all implementations into the private submodule foreign, sorting them by core, alloc and std and submodules similar to those documented in e.g. https://doc.rust-lang.org/std/, I see i.a. the following advantages:

  1. Easier to find for which types the trait is implemented, which implementations are missing,
  2. If one wants to add a implementation, one can just place it in the module according to its type definition in core/alloc/std and does not have to guess, whether to place it at the beginning or end of lib.rs or somewhere random,
  3. It will be easier to feature-gate some implementations, e.g. std for std-implementations, which should be a default-feature, which can be disabled for no_std-builds,
  4. Implementations for additional (common) crates, e.g. chrono or time, can be placed in foreign behind feature-gates,
  5. Changes to one file does not affect changes to other files, so merging multiple PRs might be less conflicting

However, this is indeed quite a large change and will require additional PRs e.g. for feature-gating, fixing the usage of std in alloc, adding missing implementations, ….

@sivizius sivizius force-pushed the restructure-implementations branch 2 times, most recently from cdf0a75 to 70dfa8f Compare August 22, 2024 12:12
At the moment, the trait-definition and all implementations for
foreign types are located in `lib.rs` with 1732 LOC. IMHO this is
difficult to maintain, especially when adding additional
implementations. By moving all implementations into the private
submodule foreign, sorting them by `core`, `alloc` and `std`, and
submodules similar to those documented, I see i.a. the following
advantages:

1. Easier to find for which types the trait is implemented, which
   implementations are missing,
2. If one wants to add a implementation, one can just place it in the
   module according to its type definition in `core`/`alloc`/`std` and
   does not have to guess, whether to place it at the beginning or end
   of `lib.rs` or somewhere random,
3. It will be easier to feature-gate some implementations, e.g. `std`
   for std-implementations, which should be a default-feature, which
   can be disabled for `no_std`-builds,
4. Implementations for additional (common) crates, e.g. `chrono` or
   `time`, can be placed in foreign behind feature-gates,
5. Changes to one file does not affect changes to other files, so
   merging multiple PRs might be less conflicting.

Somewhat unrelated changes: Harmonising trait-bounds-placement.
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@fitzgen fitzgen merged commit 915848a into rust-fuzz:main Aug 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants