feat: add uint64/base/assert/is-equal#12508
Conversation
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown_pkg_readmes
status: passed
- task: lint_markdown_docs
status: na
- task: lint_markdown
status: na
- task: lint_package_json
status: passed
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
|
/stdlib update-copyright-years |
uint64/base/assert/is-equal
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown_pkg_readmes
status: na
- task: lint_markdown_docs
status: na
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: passed
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
Coverage Report
The above coverage report was generated for the changes in this PR. |
|
@kgryte Ready for review |
| * // returns true | ||
| */ | ||
| function isEqual( a, b ) { | ||
| return ( a.hi === b.hi && a.lo === b.lo ); |
There was a problem hiding this comment.
I am thinking that we need the equivalent of complex/*/(real|imag) for resolving the high and low words. Or equivalently number/float64/base/to-words.
The reason being that we generally prefer using functional accessors rather than property access. And the main reason for this is that it makes breaking migrations much easier.
For example, suppose at some future time we decide that hi and lo were the wrong choices for property names or we decide to get rid of hi and lo altogether and have a "words" API. In order to migrate, we'd need to find every place in the codebase which uses the hi and lo properties and then update everything individually.
If, however, we have a small set of constrained APIs which handle property access, then we only have to update a small set of packages to perform the migration. Downstream code does not need to change, as they are working against a stable API abstraction, and don't need to worry about whether an instance has particular properties. Along a similar vein, providing functional accessor APIs also allows for supporting multiple different instance types, thus making it easier for downstream code to have extended support (i.e., as long the a functional API supports multiple class types, so, too, will the downstream code automatically).
In which case, before we merge this PR, I think it worth adding number/uint64/base/to-words having both a main export and an assign API, as done in https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/number/float64/base/to-words.
Similar to https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/complex/float64/reim/lib/main.js, in toWords, we can access hi and lo directly. The main export for toWords should return a "generic" array, as done in number/float64/base/to-words.
Once we have that this package can be updated to
var toWords = require( '@stdlib/number/uint64/base/to-words' ).assign;
var workspace1 = [ 0, 0 ];
var workspace2 = [ 0, 0 ];
function isEqual( a, b ) {
toWords( a, workspace1, 1, 0 );
toWords( b, workspace2, 1, 0 );
return ( a[ 0 ] === b[ 0 ] ) && ( a[ 1 ] === b[ 1 ] );
}That seem reasonable?
There was a problem hiding this comment.
Yeah, that's super insightful. I did feel toWords would be a better fit for this in the stylistic sense but would've never realized how deeply it could affect downstream support and reduce maintenance burden. I'll get to it right away.
There was a problem hiding this comment.
Thank you!
how deeply it could affect downstream support and reduce maintenance burden.
I say this because we learned this the hard way. In fact, that is why I am still doing the ndarray dtype migration. If we had avoided using ndarray#dtype directly to start with, the migration to using dtype objects would have been much much easier.
There was a problem hiding this comment.
cc @batpigandme We should document the rationale for using functional accessor APIs. This could also be packaged as a blog post. The latter would be beneficial as a means of combatting the push back from the anti-small module crowd which might have thoughts about not accessing properties directly and having accessor packages "bloat" their dep trees.
Speaking of bloat: this is another reason to use functional accessor APIs. Namely, you cannot minify foo.really_long_property_name because, unlike variable names, you have to leave property names as is (a.really_long_property_name). In which case, every time you do x.<prop_name>, you increase your bundle size by the length of <prop_name>. For very common property access, that is a not an insignificant amount of bytes. In contrast, with a functional accessor really_long_function_name, this can always be minified to a shorter symbol name (e.g., a, f, etc) during minification.
Obviously, minification is not the entire story, as bundles are often GZIP compressed when sent over the wire and repeated character sequence inflation will be mitigated, but these sequences still have impacts on physical space and in-memory usage.
So, while the anti-small module crowd loves to rail against small modules, there is more nuance than just pointing to a schematic showing a black hole for node_modules folders, and, for a project like stdlib, one can consider it a matter of best practice for using these small wrappers. That would be good to articulate more publicly.
There was a problem hiding this comment.
Also, as one final thought: functional accessors to ease migration is not a new idea. IIRC, Martin Fowler discussed such an approach in his classic book Refactoring.
Resolves none.
Description
This pull request:
Uint64instances are equal.Related Issues
No.
Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
N/A.
@stdlib-js/reviewers