Add Map properties #3505
Add Map properties #3505
Conversation
albertchen-sifive
requested review from
mikeurbach,
jackkoenig and
adkian-sifive
|
I'm going to mark this as a |
| value.map { | ||
| case (key, value) => | ||
| key -> tpe.convert(value, ctx, info) | ||
| }.toSeq.sortBy(_._1) |
There was a problem hiding this comment.
Do we actually want to be sorting here vs. just disallowing unordered maps? I think people generally want things to match the order they defined them rather than being sorted.
| type Type = F[tpe.Type] | ||
| override def getPropertyType(value: Option[F[A]]): fir.PropertyType = | ||
| fir.MapPropertyType(implicitly[PropertyType[A]].getPropertyType(None)) | ||
| type Underlying = Map[String, tpe.Underlying] |
There was a problem hiding this comment.
| type Underlying = Map[String, tpe.Underlying] | |
| type Underlying = VectorMap[String, tpe.Underlying] |
We definitely should use an ordered map here.
| implicit def propertyTypeInstance[T](implicit pte: PropertyType[T]) = new PropertyType[Property[T]] { | ||
| type Type = T | ||
| override def getPropertyType(value: Option[Property[T]]): fir.PropertyType = pte.getPropertyType(None) | ||
| override def convert(value: Underlying, ctx: ir.Component, info: SourceInfo): fir.Expression = | ||
| ir.Converter.convert(value, ctx, info) | ||
| type Underlying = ir.Arg | ||
| override def convertUnderlying(value: Property[T]) = value.ref |
There was a problem hiding this comment.
I get why this is here, but I think we should think about if this is the only way to accomplish the goal of making Property.apply work for the likes of both Property[Seq[BigInt]] and Seq[Property[BigInt]]. Is using overloading so bad since we only have the 2 collection types? I think we will just need 3 in that case.
What worries me about this is it makes it legal to write things like Property[Seq[Property[Property[Property[BigInt]]]]] and I'm not sure what that means.
There was a problem hiding this comment.
I don't think we should sandbag on this because overall this PR is a amazing and a huge step forward.
Please leave a comment explaining why this is here with maybe a TODO to see if there's either a way to remove the need for PropertyType[Property[T]] or at least disallow weird Property[Property[T]] or better yet disallow mutually recursive types like Property[Seq[Property[Seq[Property[T]]]]] (ideally disallow more than 1 Property[_] in the type if at all possible).
We can try to "fix" this later, let's definitely get this PR in.
| } | ||
|
|
||
| implicit def mapPropertyTypeInstance[A, F[A] <: Map[String, A]](implicit tpe: PropertyType[A]) = |
There was a problem hiding this comment.
| implicit def mapPropertyTypeInstance[A, F[A] <: Map[String, A]](implicit tpe: PropertyType[A]) = | |
| implicit def mapPropertyTypeInstance[A, F[A] <: SeqMap[String, A]](implicit tpe: PropertyType[A]) = |
Due to determinism issues, let's just not support Map and do SeqMap instead (using VectorMap as our concrete implementation)
| implicit def propertyTypeInstance[T](implicit pte: PropertyType[T]) = new PropertyType[Property[T]] { | ||
| type Type = T | ||
| override def getPropertyType(value: Option[Property[T]]): fir.PropertyType = pte.getPropertyType(None) | ||
| override def convert(value: Underlying, ctx: ir.Component, info: SourceInfo): fir.Expression = | ||
| ir.Converter.convert(value, ctx, info) | ||
| type Underlying = ir.Arg | ||
| override def convertUnderlying(value: Property[T]) = value.ref |
There was a problem hiding this comment.
I don't think we should sandbag on this because overall this PR is a amazing and a huge step forward.
Please leave a comment explaining why this is here with maybe a TODO to see if there's either a way to remove the need for PropertyType[Property[T]] or at least disallow weird Property[Property[T]] or better yet disallow mutually recursive types like Property[Seq[Property[Seq[Property[T]]]]] (ideally disallow more than 1 Property[_] in the type if at all possible).
We can try to "fix" this later, let's definitely get this PR in.
| @@ -51,22 +51,63 @@ private[chisel3] trait PropertyType[T] { | |||
| def convertUnderlying(value: T): Underlying | |||
| } | |||
|
|
|||
| private[chisel3] trait RecursivePropertyType[T] extends PropertyType[T] | |||
There was a problem hiding this comment.
A comment describing what this is doing would be good.
| private[chisel3] trait LowPriorityPropertyTypeInstances { | ||
| implicit def sequencePropertyTypeInstance[A, F[A] <: Seq[A]](implicit tpe: RecursivePropertyType[A]) = | ||
| new SeqPropertyType[A, F, tpe.type](tpe) with RecursivePropertyType[F[A]] | ||
|
|
||
| implicit def mapPropertyTypeInstance[A, F[A] <: Map[String, A]](implicit tpe: RecursivePropertyType[A]) = | ||
| new MapPropertyType[A, F, tpe.type](tpe) with RecursivePropertyType[F[A]] | ||
| } |
There was a problem hiding this comment.
A comment describing why this is necessary would be helpful
Contributor Checklist
docs/src?Type of Improvement
This PR changes the
Property.applyfromdef apply[T: PropertyType](): Property[T]todef apply[T]()(implicit tpe: PropertyType[T]): Property[tpe.Type]. This lets implicit resolution resolve the result type, instead of having to add different apply methods for all the combinations ofTandProperty[T]alternatives -Seq[Property[T]]/Seq[T],Map[String, Property[T]]/Map[String, T],(Property[A], Property[B])/(A, B)etc.Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.5.x,3.6.x, or5.xdepending on impact, API modification or big change:6.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.