buffer,fs: support immutable arraybuffers - #66379
avivkeller wants to merge 1 commit into
Conversation
Signed-off-by: avivkeller <me@aviv.sh>
nodejs-github-bot
added
lib / src
| // NOLINTNEXTLINE(runtime/references) | ||
| FastApiCallbackOptions& options) { | ||
| TRACK_V8_FAST_API_CALL("buffer.isImmutable"); | ||
| HandleScope scope(options.isolate); |
There was a problem hiding this comment.
Does this really need a handle scope?
|
Actually, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66379 +/- ##
=======================================
Coverage 90.35% 90.36%
=======================================
Files 792 792
Lines 275434 275492 +58
Branches 52780 52796 +16
=======================================
+ Hits 248878 248947 +69
+ Misses 16981 16964 -17
- Partials 9575 9581 +6
🚀 New features to boost your workflow:
|
| bool IsImmutableImpl(Local<Value> view) { | ||
| return view.As<ArrayBufferView>()->Buffer()->IsImmutable(); | ||
| } |
There was a problem hiding this comment.
| bool IsImmutableImpl(Local<Value> view) { | |
| return view.As<ArrayBufferView>()->Buffer()->IsImmutable(); | |
| } | |
| bool IsImmutableImpl(Local<Value> value) { | |
| Local<ArrayBufferView> abv = value.As<ArrayBufferView>(); | |
| if (!abv->HasBuffer()) { | |
| return false; | |
| } | |
| return abv->Buffer()->IsImmutable(); | |
| } |
Buffer() has side-effects if a typed array was created with on-heap storage.
There was a problem hiding this comment.
This should likely also at least DCHECK that view->IsArrayBufferView()
There was a problem hiding this comment.
Actually, given how this is used, I think you actually need a tri-state return.
For the IsImmutableImpl, return an std::optional<bool>.
std::nullopt if view is not an ArrayBufferView.
false if view is not immutable.
true if view is immutable.
Then at the callsites, std::nullopt should map to null
(that's a bit pedantic tho so I'll leave it up to you... at a minimum the DCHECK)
| validateBuffer(buffer); | ||
| } | ||
|
|
||
| validateWritableBuffer(buffer); |
There was a problem hiding this comment.
Bikeshed: I'd prefer validateMutableBuffer but non-blocking.
6 participants
Ref: #66346
Adds error-handling for immutable ArrayBuffers in the
node:fsmodule + support for these ArrayBuffers in Node'sBuffer.cc @panva @jasnell
Note that
%TypedArray%.prototype.set()rejects immutable ArrayBuffers, so I've updated the buffer implementation to use_copyinstead.