-
Notifications
You must be signed in to change notification settings - Fork 51
Topology ports semantics #855
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
base: feature/topology-ports
Are you sure you want to change the base?
Conversation
…logy-ports-semantics
…ology-ports-semantics
…ology-ports-semantics
…ology-ports-semantics
…ology-ports-semantics
| override def getUnqualifiedName = node._2.data.name | ||
| } | ||
|
|
||
| type InterfaceInstance = Symbol.ComponentInstance | Symbol.Topology |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, for consistency, let's make InterfaceInstance a sealed trait if possible.
| c.out -> c.in | ||
| ^ | ||
| error: out is not a port instance of component C | ||
| error: out is not a port instance of interface c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Should this error message say
interface instance c? - Can we provide a note here saying that c has type C? Otherwise there is a small regression in the quality of the error output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general for this kind of error, c has to be a topology T, a component instance with component type C, or an interface parameter with interface I, right? Could the error message say "... is not a port instance of topology T/component C/interface I"? That would be clearer and would preserve what we had before.
| ^ | ||
| error: symbol c is not defined | ||
| note: looking for a component instance here | ||
| note: looking for a port interface instance here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this error message say "looking for a component instance, topology, or interface parameter here"? I think that would be clearer.
| match p1 with p2 | ||
| ^ | ||
| error: p2 is not a port instance of component C | ||
| error: p2 is not a port instance of interface C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change to the error message doesn't look right. A matching specifier occurs in a component definition, not an interface definition.
| loc: Location // The location whether the connection is requested | ||
| ): Result.Result[Unit] = Right(()) | ||
|
|
||
| override def equals(obj: Any): Boolean = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to override equals here? What happens if we use the default equals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I just have a few comments/questions about error messages and coding.
…ology-ports-semantics
This PR implements topology port semantics according to the spec from #748
Interesting Changes
interfaceInstanceUseportMap+specialPortMapwere moved out into a separatePortInterfaceconstruct so that it could be reused inside topologyPortInstance.Topologyfor wrapping ports with a new name to impement topology ports.Closes #745