tlv: add new types and functions #8692
Conversation
In this commit, we add a new type alias for a blob type. This type can be used in areas where a byte slice is used to store a TLV value, which may be a fully opaque nested TLV.
This type is useful when one wants to encode an integer as an underlying BigSize record. It wraps any integer, then handles the transformation into and out of the BigSize encoding on disk.
ValOpt returns an Option of the underlying value.
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| // Blob is a type alias for a byte slice. It's used to indicate that a slice of | ||
| // bytes is actually an encoded TLV stream. | ||
| type Blob = []byte |
There was a problem hiding this comment.
If there are different semantics here that are being assumed, I'd recommend not making it a simple alias and actually make a newtype (dropping the =)
There was a problem hiding this comment.
In this case using the type alias is useful, as then I can put it in a tlv.RecordT as a primitive type. Otherwise, you'd need the record method wrapper.
There was a problem hiding this comment.
We should just make RecordT a primitive type, no? I just want to stress that type aliases are not useful for anything other than giving the reader a hint on how something is used, it assures no correct api usage the way that a newtype wrapper does.
|
|
||
| // BigSizeT is a high-order type that represents a TLV record that encodes an | ||
| // integer as a BigSize value in the stream. | ||
| type BigSizeT[T constraints.Integer] struct { |
| // record. | ||
| func (o *OptionalRecordT[T, V]) ValOpt() fn.Option[V] { | ||
| return fn.MapOption(func(record RecordT[T, V]) V { | ||
| return record.Val |
There was a problem hiding this comment.
Pondering...
This makes me wonder if we should extract out Val as a function. We have the following issue here:
-- we have these
type OptionalRecord t v = Option (Record t v)
valOpt :: Option (Record t v) -> Option v
mapOption :: (a -> b) -> Option a -> Option b
-- if we had this
val :: Record t v -> v
-- then we could do this
valOpt :: OptionalRecord t v -> Option v
valOpt = mapOption valPorting this go to is tricky since there's no (to my knowledge) easy way for us to refer to a method without yet supplying its receiver argument. So defining something like this wouldn't fix the ergonomics:
func (r RecordT[T, V]) Val() V { ... }
func (o *OptionalRecordT[T, V]) ValOpt() fn.Option[V] {
return fn.MapOption(RecordT.Val)(*o) // no sir
}
// but we could do this
func Val[T, V any](r Record[T, V]) V { ... }
// and then do this
func (o *OptionalRecordT[T, V] ValOpt() fn.Option[V] {
return fn.MapOption(Val)(*o) // can do
}I say all this because if we have to create new names for things every time we want to map a function over some functor, then Map<FUNCTOR> loses a lot of its luster.
If you have ideas and/or opinions here, lmk. This code is fine, it just points to the fact that there's room for improvement on our functional patterns.
|
Pushed a new tag: |
Extracted from #8683 and #8684.
Will be helpful to have this merged to
masterso we can reference the commit directly, without needing to use ago.workfile (which then would require us to bump to Go 1.22).