Revert "const-oid: add ObjectIdentifierRef; use for db (#1212)"
#1299
Conversation
This file contains hidden or 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
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
Previously the v0.10-pre release series has attempted to make `ObjectIdentifier` generic around a backing buffer containing the BER encoding and bounded on `AsRef<[u8]>`. This approach has a drawback though: we can't use derived `PartialEq`/`Eq`, which means it isn't possible to use in `match` expressions anymore, an ergonomics drawback noted in #1293, with the implementation reverted in #1299. An alternative way to go for what it was trying to implement: an `ObjectIdentifierRef` backed by a `&[u8]` is to use a separate struct. With that approach, there could be overlapping `PartialEq`/`Eq` impls that would allow the two to be compared. As a start towards going that route, this gets rid of the generic backing buffer and opts instead to make the struct const generic around its size with a default.
tarcieri
added a commit
that referenced
this pull request
Jan 4, 2024
Previously the v0.10-pre release series has attempted to make `ObjectIdentifier` generic around a backing buffer containing the BER encoding and bounded on `AsRef<[u8]>`. This approach has a drawback though: we can't use derived `PartialEq`/`Eq`, which means it isn't possible to use in `match` expressions anymore, an ergonomics drawback noted in #1293, with the implementation reverted in #1299. An alternative way to go for what it was trying to implement: an `ObjectIdentifierRef` backed by a `&[u8]` is to use a separate struct. With that approach, there could be overlapping `PartialEq`/`Eq` impls that would allow the two to be compared. As a start towards going that route, this gets rid of the generic backing buffer and opts instead to make the struct const generic around its size with a default.
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.
This reverts commit 2f6303b.
Per #1293 to keep the ergonomics of the v0.9 release we need derived
Eq/PartialEqimpls which allow the constants to be used inmatchexpressions.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
ObjectIdentifierRefis 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.