crypto: refactor array buffer view validation#29683
crypto: refactor array buffer view validation#29683BridgeAR wants to merge 2 commits intonodejs:masterfrom
Conversation
This is just a refactoring to reduce code and computational overhead.
nodejs-github-bot
added
the
lib / src
| if (typeof data !== 'string' && !isArrayBufferView(data)) { | ||
| throw invalidArrayBufferView('data', data); | ||
| throw new ERR_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
|
CI passed, if that encourages anyone to take a closer look. |
BridgeAR
added
the
author ready
Trott
removed
the
author ready
|
Removed |
|
Also: Should this be benchmarked? |
|
@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 <jasnell@gmail.com>
|
Landed in eb05d68 |
This is just a refactoring to reduce code and computational overhead. PR-URL: #29683 Reviewed-By: James M Snell <jasnell@gmail.com>
4 participants
This is just a refactoring to reduce code and computational overhead.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes