ConcreteFunction fix and performance improvements #364
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
karllessard
force-pushed
the
ryan-rn-func-fix
branch
from
7c48738 to
424fe94
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@rnett , looks like you'll need to consent my new PR... |
| */ | ||
| public Map<String, Operand<?>> call(Scope scope, Map<String, Operand<?>> arguments) { | ||
| List<Operand<?>> inputList = new ArrayList<>(signature.inputNames().size()); | ||
| List<Operand<?>> inputList = new ArrayList<>(signature().inputNames().size()); |
There was a problem hiding this comment.
Why does this go through the signature accessor now? If we want to do that, would it be better to buffer the signature in a local variable to avoid all the times signature() is called?
There was a problem hiding this comment.
I've looked and didn't see any reason for calling signature() instead of signature, since there are no class extending ConcreteFunction that I'm aware of. I've marked the class as final to make sure of that.
|
|
||
| if (i > outputList.size()) { | ||
| throw new IllegalStateException( | ||
| "Somehow, not all required outputs were returned from the function"); |
There was a problem hiding this comment.
Might be useful if this exception returned both i and outputList.size(). Though I suspect it's very unlikely to happen.
There was a problem hiding this comment.
Done. I've also move that sanity check outside the loop, it could be done beforehand.
Signed-off-by: Ryan Nett <[email protected]>
karllessard
force-pushed
the
ryan-rn-func-fix
branch
from
424fe94 to
429f66b
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
|
Signed-off-by: Ryan Nett [email protected]
Rebase of #360