-
-
Notifications
You must be signed in to change notification settings - Fork 837
feat: add array/float16
#7435
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: develop
Are you sure you want to change the base?
feat: add array/float16
#7435
Conversation
/stdlib update-copyright-years |
Hello @Planeshifter and @kgryte sir, So, i can start working on adding float16 datatype support for |
|
||
// MAIN // | ||
|
||
class Float16Array { |
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 entire implementation should be refactored. We do not use ES6+. All code must be written in ES5. Furthermore, all methods and properties should be documented. See array/bool
and array/complex128
for what we expect.
return Float16Array.fromFloat16(this._buffer[index]); | ||
} | ||
|
||
get buffer() { |
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 file also does not conform to our style conventions and includes mixed tabs and spaces. Please follow our development guide and ensure that your local environment is properly setup. Our automated linting and EditorConfig configuration would have flagged all these issues.
const offset = value !== undefined ? Number(value) : 0; | ||
|
||
if (offset < 0 || offset > this.length) { | ||
throw new RangeError('Offset is out of bounds'); |
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 is not how we format error messages.
(ArrayBuffer.isView(source) && typeof source.length === 'number') || | ||
(source instanceof Float16Array)) | ||
) { | ||
const len = Math.min(source.length, this.length - offset); |
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.
We do not use Math
built-ins. Everything should be implemented in terms of stdlib functionality.
return this._entriesGenerator(); | ||
} | ||
|
||
*_entriesGenerator() { |
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.
entries
should be implemented using iterators directly. Use of generators is not allowed in ES5.
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.
@udaykakade25 Thank you for working on this. I left a few initial comments. The TL;DR is that the entire implementation needs to be reworked to be implemented in ES5. I strongly encourage you to emulate the style and approach used in array/bool
and other custom array types implemented in stdlib.
progresses #7347 and #7273
Description
This pull request:
Related Issues
This pull request:
assert/is-float16array
#7273 feat: addassert/has-float16array-support
#7347Questions
No.
Other
This is the third PR. First two PRs are interconnected to each other. In order to merge this PR, we need to first merge PR 1: #7273 and #7347
Checklist
@stdlib-js/reviewers