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

fixed an error in the default method for PositionNot #1672

Merged
merged 1 commit into from
Sep 7, 2017

Conversation

ThomasBreuer
Copy link
Contributor

fixed an error in the default method for PositionNot (and PositionNonZero),
in the case that the given 'from' value is larger than the length of the list

@markuspf
Copy link
Member

markuspf commented Sep 6, 2017

This seems to be breaking alghom.tst.

lib/kernel.g Outdated
@@ -168,7 +168,7 @@ POSITION_NOT := function( arg )
return i;
fi;
od;
return LENGTH(arg[1]) + 1;
return arg[3] + 1;
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite correct: Think about arg[3] being 0 (in fact, PositionNot(list,x) is defined as PositionNot(list,x,0).

So instead of changing this line, you could insert this before line 166:

if arg[3]+1 > LENGTH(arg[1]) then
    return arg[3] + 1;
fi;

@codecov
Copy link

codecov bot commented Sep 7, 2017

Codecov Report

Merging #1672 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1672      +/-   ##
==========================================
+ Coverage   64.42%   64.42%   +<.01%     
==========================================
  Files        1002     1002              
  Lines      326709   326712       +3     
  Branches    13197    13218      +21     
==========================================
+ Hits       210477   210486       +9     
+ Misses     113363   113357       -6     
  Partials     2869     2869
Impacted Files Coverage Δ
lib/kernel.g 66.66% <100%> (+1.58%) ⬆️
src/range.c 87.39% <0%> (-0.22%) ⬇️
src/hpc/threadapi.c 34.36% <0%> (-0.1%) ⬇️
src/gap.c 57.24% <0%> (+0.08%) ⬆️
src/system.c 53.1% <0%> (+0.33%) ⬆️
src/hpc/thread.c 47.03% <0%> (+0.39%) ⬆️
src/funcs.c 72.66% <0%> (+0.4%) ⬆️

(and PositionNonZero), in the case that the given 'from' value
is larger than the length of the list
@ThomasBreuer
Copy link
Contributor Author

just fixed the fix, thanks for the hints

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Code looks good now, let's wait for the test, though

@fingolfin fingolfin dismissed their stale review September 7, 2017 08:59

The required changes were made

@markuspf markuspf merged commit 6532c37 into gap-system:master Sep 7, 2017
@olexandr-konovalov olexandr-konovalov added the gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall label Sep 7, 2017
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 22, 2018
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants