-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Represent non-fixed params as NaN for QkTargetOp
#15463
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: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 20375166055Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Cryoris
left a comment
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.
From a bugfix POV I don't think we need to backport this (since we can't create a C Target object that would have params of unknown length). But from an interface POV this PR changes an all-free params from a NULL pointer to an array of f64::NAN and it would be good to not change this interface between 2.3 and 2.4, which makes it backportable IMO.
crates/cext/src/transpiler/target.rs
Outdated
| /// it will be represented as with the `NaN` value. If there are no parameters | ||
| /// then this value will be represented with a `NULL` pointer. |
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 last part isn't true anymore, is it? If all parameters are free then we'd still have an array with all f64::NAN entries, right?
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.
I guess I could've worded it better. But what I meant was if the operation doesn't accept any parameters.
We pivoted the approach, after internal discussion, to use `QkParam` instead.
- Since the `QkTarget` instance owns the params. We should be able to retrieve the operation's parameters without cloning by turning references into pointers.
|
One or more of the following people are relevant to this code:
|
Summary
Fixes #15438
Due to our decision of provisionally exposing parameters in
QkTargetOpasdouble[]we mistakenly filtered out any non-fixed parameters instead of showing a placeholder value.This PR provisionally exposes any non-fixed parameter instance asThis PR now exposesNaNuntil we work out a much cleaner way of exposing the parameters to C.QkParamdirectly by having theparamsattribute be an array ofQkParam.Details and comments