Skip to content

Conversation

aayush0325
Copy link
Member

Resolves #3144

Description

What is the purpose of this pull request?

This pull request:

  • adds findLastIndex method to array/fixed-endian-factory
  • adds relevant tests and benchmarks
  • updates README.md with appropriate documentation.

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot
Copy link
Contributor

stdlib-bot commented Nov 29, 2024

Coverage Report

Package Statements Branches Functions Lines
array/fixed-endian-factory $\color{red}1163/1414$
$\color{green}+82.25\%$
$\color{red}150/162$
$\color{green}+92.59\%$
$\color{red}23/33$
$\color{green}+69.70\%$
$\color{red}1163/1414$
$\color{green}+82.25\%$

The above coverage report was generated for the changes in this PR.

@aayush0325
Copy link
Member Author

kindly give this a review @kgryte!

@aayush0325
Copy link
Member Author

@Planeshifter I've resolved the merge conflicts in this branch

@aayush0325
Copy link
Member Author

aayush0325 commented Dec 1, 2024

this branch should be free of any indentation errors which were made while resolving merge conflicts now.

@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Dec 3, 2024
* @returns {boolean} boolean indicating whether a value passes a test
*/
function predicate( value ) {
return value % 2 === 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should always evaluate to false. E.g., value < 0.0. Otherwise, the benchmark is effectively O(1), which is not what we want.

/**
* Returns the index of the last element in an array for which a predicate function returns a truthy value.
*
* @name findLastIndex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @name findLastIndex
* @private
* @name findLastIndex

Comment on lines 661 to 671
var len;
var i;
if ( !isTypedArray( this ) ) {
throw new TypeError( format( 'invalid invocation. `this` is not %s %s.', CHAR2ARTICLE[ dtype[0] ], CTOR_NAME ) );
}
if ( !isFunction( predicate ) ) {
throw new TypeError( format( 'invalid argument. First argument must be a function. Value: `%s`.', predicate ) );
}
buf = this._buffer;
len = this._length;
for ( i = len - 1; i >= 0; i-- ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var len;
var i;
if ( !isTypedArray( this ) ) {
throw new TypeError( format( 'invalid invocation. `this` is not %s %s.', CHAR2ARTICLE[ dtype[0] ], CTOR_NAME ) );
}
if ( !isFunction( predicate ) ) {
throw new TypeError( format( 'invalid argument. First argument must be a function. Value: `%s`.', predicate ) );
}
buf = this._buffer;
len = this._length;
for ( i = len - 1; i >= 0; i-- ) {
var i;
if ( !isTypedArray( this ) ) {
throw new TypeError( format( 'invalid invocation. `this` is not %s %s.', CHAR2ARTICLE[ dtype[0] ], CTOR_NAME ) );
}
if ( !isFunction( predicate ) ) {
throw new TypeError( format( 'invalid argument. First argument must be a function. Value: `%s`.', predicate ) );
}
buf = this._buffer;
for ( i = this._length - 1; i >= 0; i-- ) {

The local variable is not necessary in this case, as we are only accessing _length once.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aayush0325 Thanks for working on this. Left a few comments. Once resolved, this should be ready for merge, provided that the merge conflicts are resolved.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Dec 3, 2024
@aayush0325 aayush0325 requested a review from kgryte December 4, 2024 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC]: add findLastIndex method to array/fixed-endian-factory

3 participants