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

kernel: unify 'and' for filters in interpreter and executor #2755

Closed
wants to merge 2 commits into from

Conversation

fingolfin
Copy link
Member

Also start using IS_FILTER. Note that previously, this input was silently accepted:

gap> Center and IsAssociative;
<Filter "<<and-filter>>">

This was effectively treated equivalently to HasCenter and IsAssociative;.
We now could reject such code. But unfortunately a few packages accidentally
relied on this bug. We thus only print a warning for now, and will turn this
into a proper error only once all affect packages released a fixed version.

This continues the work begun in PR #2732

@fingolfin fingolfin added topic: kernel release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 28, 2018
Error, <expr> must be 'true' or 'false' (not a function)
gap> function() return Center and IsAssociative; end();
Warning, operation 'Centre' used in AND-filter is not a filter in stream:1
<Filter "(Centre and IsAssociative)">
Copy link
Member Author

Choose a reason for hiding this comment

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

Here the warning can be seen in effect

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2755      +/-   ##
==========================================
+ Coverage   76.03%   76.03%   +<.01%     
==========================================
  Files         480      480              
  Lines      240919   240946      +27     
==========================================
+ Hits       183185   183214      +29     
+ Misses      57734    57732       -2
Impacted Files Coverage Δ
src/opers.h 100% <100%> (ø) ⬆️
src/error.c 80.15% <100%> (+0.48%) ⬆️
src/intrprtr.c 98.64% <100%> (ø) ⬆️
src/exprs.c 97.64% <100%> (+1.05%) ⬆️
src/stringobj.c 93.31% <0%> (-0.64%) ⬇️
src/hookintrprtr.c 67.34% <0%> (+1.02%) ⬆️

@ChrisJefferson
Copy link
Contributor

I wonder if we should not merge this until after 4.10 branches, to give packages a full release cycle to fix bugs, rather than have warnings in 4.10.1? Or is it important those bugs get fixed ASAP?

@fingolfin
Copy link
Member Author

No, it's not important at all. I just suggested it as a possibility in PR #2732, and @ChrisJefferson wrote:

If 2 isn't too much work, it quite appeals, because it also means this doesn't get lost.

;-). But I am also fine with skipping this PR completely, and just merging PR #2756 (squashed down) once all affected packages made a fixed release.

Also start using IS_FILTER. Note that previously, this input was silently accepted:

  gap> Center and IsAssociative;
  <Filter "<<and-filter>>">

This was effectively treated equivalently to `HasCenter and IsAssociative;`.
We now could reject such code. But unfortunately a few packages accidentally
relied on this bug. We thus only print a warning for now, and will turn this
into a proper error only once all affect packages released a fixed version.
@fingolfin
Copy link
Member Author

Hmm, just realized that there should be a third place that needs to be adjusted: the compiler. Gotta do that, including a test

@fingolfin
Copy link
Member Author

Closing this in favor of PR #2756

@fingolfin fingolfin closed this Oct 13, 2018
@fingolfin fingolfin deleted the mh/stricter-and-filter branch October 13, 2018 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants