-
Notifications
You must be signed in to change notification settings - Fork 134
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
const-oid: add ObjectIdentifierRef
; use for db
#1212
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a type alias for `ObjectIdentifier<&[u8]>` for OIDs whose backing storage is a byte slice. Changes the `db` to use OIDs backed by `'static` byte slices, which makes them smaller than if they use a fixed-size buffer. Also makes more methods generic so e.g. `Eq`/`PartialEq` work between `ObjectIdentifier` and `ObjectIdentifierRef`.
It might make sense to change the database from Edit: done in d16bd0b |
Hmm, there is one big disadvantage to d16bd0b: statics can't be used in constants, so you couldn't reference OIDs from the database in other constants. Going to revert that for now and we can consider it separately. |
tarcieri
force-pushed
the
const-oid/object-identifier-ref
branch
from
September 4, 2023 00:20
d16bd0b
to
cac1d55
Compare
tarcieri
added a commit
that referenced
this pull request
Jan 4, 2024
This reverts commit 2f6303b. Per #1293 to keep the ergonomics of the v0.9 release we need derived `Eq`/`PartialEq` impls which allow the constants to be used in `match` expressions. That's not really possible with the generic backing storage approach that the v0.10 prereleases have been trying, and also precludes comparing OIDs with different backing storages. While an `ObjectIdentifierRef` is still worth experimenting with, it's clear it isn't ready for prime time yet, so this begins the process of backing out some changes so the database isn't yet dependent on its existence.
tarcieri
added a commit
that referenced
this pull request
Jan 4, 2024
…1299) This reverts commit 2f6303b. Per #1293 to keep the ergonomics of the v0.9 release we need derived `Eq`/`PartialEq` impls which allow the constants to be used in `match` expressions. That's not really possible with the generic backing storage approach that the v0.10 prereleases have been trying, and also precludes comparing OIDs with different backing storages. While an `ObjectIdentifierRef` is still worth experimenting with, it's clear it isn't ready for prime time yet, so this begins the process of backing out some changes so the database isn't yet dependent on its existence.
tarcieri
added a commit
that referenced
this pull request
Jan 5, 2024
Adds a `repr(transparent)` newtype for a `[u8]` which is guaranteed to contain a valid BER serialization of an OID. This is a similar approach to how `Path`/`PathBuf` or `OsStr`/`OsString` work (except with `ObjectIdentifier` being stack-allocated instead of heap allocated). An unsafe pointer cast is required to go from `&[u8]` to `&ObjectIdentifierRef`, so unfortunately this means the crate is no longer `forbid(unsafe_code)`, however it's been lowered to `deny(unsafe_code)` to ensure contributors think twice before adding more. `Borrow` and `Deref` impls have been added to the owned `ObjectIdentifier` type, allowing common functionality to be moved to `ObjectIdentifierRef`, allowing both types to exist while eliminating code duplication. A `PartialEq` impl allows them to be compared. The `db` module continues to use `ObjectIdentifier` for now, however hopefully this approach would allow #1212 to be reinstated and for `ObjectIdentifierRef`s to be used for the database eventually (i.e. revert the revert in #1299)
tarcieri
added a commit
that referenced
this pull request
Jan 5, 2024
Adds a `repr(transparent)` newtype for a `[u8]` which is guaranteed to contain a valid BER serialization of an OID. This is a similar approach to how `Path`/`PathBuf` or `OsStr`/`OsString` work (except with `ObjectIdentifier` being stack-allocated instead of heap allocated). An unsafe pointer cast is required to go from `&[u8]` to `&ObjectIdentifierRef`, so unfortunately this means the crate is no longer `forbid(unsafe_code)`, however it's been lowered to `deny(unsafe_code)` to ensure contributors think twice before adding more. `Borrow` and `Deref` impls have been added to the owned `ObjectIdentifier` type, allowing common functionality to be moved to `ObjectIdentifierRef`, allowing both types to exist while eliminating code duplication. A `PartialEq` impl allows them to be compared. The `db` module continues to use `ObjectIdentifier` for now, however hopefully this approach would allow #1212 to be reinstated and for `ObjectIdentifierRef`s to be used for the database eventually (i.e. revert the revert in #1299) NOTE: this PR also relaxes the previous requirement that an OID have at least three arcs. It is now allowed to only have two.
tarcieri
added a commit
that referenced
this pull request
Jan 5, 2024
Adds a `repr(transparent)` newtype for a `[u8]` which is guaranteed to contain a valid BER serialization of an OID. This is a similar approach to how `Path`/`PathBuf` or `OsStr`/`OsString` work (except with `ObjectIdentifier` being stack-allocated instead of heap allocated). An unsafe pointer cast is required to go from `&[u8]` to `&ObjectIdentifierRef`, so unfortunately this means the crate is no longer `forbid(unsafe_code)`, however it's been lowered to `deny(unsafe_code)` to ensure contributors think twice before adding more. `Borrow` and `Deref` impls have been added to the owned `ObjectIdentifier` type, allowing common functionality to be moved to `ObjectIdentifierRef`, allowing both types to exist while eliminating code duplication. A `PartialEq` impl allows them to be compared. The `db` module continues to use `ObjectIdentifier` for now, however hopefully this approach would allow #1212 to be reinstated and for `ObjectIdentifierRef`s to be used for the database eventually (i.e. revert the revert in #1299) NOTE: this PR also relaxes the previous requirement that an OID have at least three arcs. It is now allowed to only have two.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a type alias for
ObjectIdentifier<&[u8]>
for OIDs whose backing storage is a byte slice.Changes the
db
to use OIDs backed by'static
byte slices, which makes them smaller than if they use a fixed-size buffer.Also makes more methods generic so e.g.
Eq
/PartialEq
work betweenObjectIdentifier
andObjectIdentifierRef
.