crypto: refactor array buffer view validation#29683
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This is just a refactoring to reduce code and computational overhead.
nodejs-github-bot added the lib / src 
| if(typeofdata!=='string'&&!isArrayBufferView(data)){ | ||
| throwinvalidArrayBufferView('data',data); | ||
| thrownewERR_INVALID_ARG_TYPE( | ||
| 'data',['string','Buffer','TypedArray','DataView'],data); |
There was a problem hiding this comment.
@nodejs/crypto all other cases that validate the input like that convert strings to buffers before passing the data on. Is it intentional, that this is not done here?
There was a problem hiding this comment.
@BridgeAR Should we pull the author ready label until we get an answer for this question? Or are you comfortable landing as-is?
There was a problem hiding this comment.
I am comfortable to land this as-is. It's mainly about consistency and the behavior is not changed with this PR.
BridgeAR requested review from bnoordhuis, cjihrig, danbev, sam-github and tniessen
nodejs-github-bot commented Sep 24, 2019
Trott commented Sep 25, 2019
CI passed, if that encourages anyone to take a closer look. |
BridgeAR added the author ready 
Trott removed the author ready Trott commented Oct 1, 2019
Removed |
Trott commented Oct 1, 2019
Also: Should this be benchmarked? |
BridgeAR commented Oct 1, 2019
@Trott this should not have any real performance impact. It might actually be faster due to some redundant typeof checks being removed. |
This is just a refactoring to reduce code and computational overhead. PR-URL: #29683 Reviewed-By: James M Snell <[email protected]>
BridgeAR commented Oct 1, 2019
Landed in eb05d68 |
This is just a refactoring to reduce code and computational overhead. PR-URL: #29683 Reviewed-By: James M Snell <[email protected]>
4 participants
This is just a refactoring to reduce code and computational overhead.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes