Skip to content
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

first and last correctly uses affix sizes #65

Merged
merged 1 commit into from
Sep 2, 2018

Conversation

paldepind
Copy link
Member

first and last didn't use the stored size of the prefix and suffix when accessing the prefix and suffix. This broke operations that rely on the affix size: prepend and append which mutate the affix and slice which changes the affix sizes to slice.

The change in this PR fixes the bug in first and last. An alternative fix would be to simply define first and last based on nth like this:

function first(l) {
  return nth(0, l);
}

That is simpler but would likely be slightly slower than the specialized first and last implementation. What do you think about those two options @emmanueltouzery?

Fixes #64.

@emmanueltouzery
Copy link
Contributor

so, the new first looks like that:

  const prefixSize = getPrefixSize(l);
  return prefixSize !== 0
    ? l.prefix[prefixSize - 1]
    : l.length !== 0
      ? l.suffix[0]
      : undefined;

and nth:

export function nth<A>(index: number, l: List<A>): A | undefined {
  if (index < 0 || l.length <= index) {
    return undefined;
  }
  const prefixSize = getPrefixSize(l);
  const suffixSize = getSuffixSize(l);
  if (index < prefixSize) {
    return l.prefix[prefixSize - index - 1];
  } else if (index >= l.length - suffixSize) {
    return l.suffix[index - (l.length - suffixSize)];
}

nth doesn't seem much slower for first and last. we could measure it.. but in the end it's your call.

@paldepind
Copy link
Member Author

nth doesn't seem much slower for first and last.

Yes, I agree. I'm going to leave the specialized first and last in place. But, if they need to be changed again I'll likely turn them into nth(0).

@paldepind paldepind merged commit b40eb2c into master Sep 2, 2018
@paldepind paldepind deleted the first-and-last-affix-sizes branch September 2, 2018 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bugs: list returns wrong results (likely bug in drop function)
2 participants