-
Notifications
You must be signed in to change notification settings - Fork 613
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
[util] Add a withShadowLayer Queue #4589
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
55a1a1e
[scala3] Fix Scala3 difference in Probe API
seldridge f0a4bda
[util] Add a withShadowLayer Queue
seldridge d3d9908
fixup! [util] Add a withShadowLayer Queue
seldridge 3558bae
fixup! [util] Add a withShadowLayer Queue
seldridge a56480f
fixup! [util] Add a withShadowLayer Queue
seldridge dbe7246
fixup! [util] Add a withShadowLayer Queue
seldridge a0d5825
fixup! [util] Add a withShadowLayer Queue
seldridge 035922a
fixup! [util] Add a withShadowLayer Queue
seldridge 790eb2d
fixup! [util] Add a withShadowLayer Queue
seldridge 424c92e
fixup! [util] Add a withShadowLayer Queue
seldridge 3ad2125
fixup! [util] Add a withShadowLayer Queue
seldridge 95a3370
fixup! [util] Add a withShadowLayer Queue
seldridge 82c46c2
fixup! [util] Add a withShadowLayer Queue
seldridge File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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 file contains 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 file contains 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
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.
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.
Shouldn't this have two data types? Shadow is probably not the same type as T...?
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.
Interestingly, no, due to how
ShadowFactory
works. AShadowFactory
takes a type parameter that is the same as the queue it was created from. However, it has a method that has a different type parameter that is the type of the shadow.I.e., if you start with a
Queue[UInt]
, you get aShadowFactory[UInt]
. However, that has a methodapply[A <: Data]
whereA
is the type of the new shadow queue. ThisA
isn't fixed, however. I.e., if you create aShadowFactory
, you then want to create anA
that can be aUInt
,SInt
, or any user-definedBundle
.If
withShadow
took a second type parameter, then it would be locked in to creating shadow queues of only that type (or it would be something overly pessimistic likeData
requiringasTypeOf
casts). (I originally ran into this problem and switched this to theShadowFactory
approach to avoid it!)Sound good?
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.
sure, I agree with what you've said that we don't need to pass the desired shadow type through, though it still seems like the ShadowFactory type might as well just be
Data
... I am not sure how the fact that the original queue was of typeT
is used at all by the code in ShadowFactory? not blockingThere 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.
oh! i see you have changed this so that
ShadowFactory
no longer takes the T parameter 👍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.
Yeah, your intuition was right here in that it doesn't need the type parameter at all. That was a nice suggestion.