Skip to content
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

Relax master parameter of RocketCrossingParams #2634

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Conversation

jsmithsf
Copy link
Contributor

Related issue:

Type of change: other enhancement

Impact: API modification

Development Phase: implementation

Release Notes

This change is meant to allow for using more generic or even specialized implementations of the master parameter of RocketCrossingKey.

@hcook I'm a little bit more concerned about my change to subsystem/Configs.scala. Right now just TileMasterPortParams is used. If another subclass gets used then WithIncoherentTiles might not do the right thing. I wasn't sure if this was the best approach.

@jsmithsf jsmithsf requested a review from hcook September 11, 2020 02:14
@hcook
Copy link
Member

hcook commented Sep 11, 2020

Yeah, an unfortunate consequence of relaxing the type is the loss of guarantee that there exists a notional cache cork to add or remove. But the WithIncoherentTiles Config alteration is also already specific to RocketCrossingKey anyway... I think it has to be up to the SoC integrator to ensure that their equivalent of such an alteration contains the full cross product of tile crossing Fields x TilePortParams subclasses that they might be exposed to, and what to do for each combination.

A more conservative implementation would be to throw an exception upon encountering a TilePortParamsLike subclass that the alteration doesn't understand.

Throw exception on unrecognized TileMasterPortParams subtype
@jsmithsf
Copy link
Contributor Author

@hcook does this look more acceptable?

@jsmithsf jsmithsf merged commit 07e158e into master Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants