Skip to content

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
@fingolfin fingolfin force-pushed the mh/stricter-and-filter branch from a8008b3 to 2b48e81 Compare August 28, 2018 10:38
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 fingolfin force-pushed the mh/stricter-and-filter branch from 2b48e81 to 54c34b7 Compare September 12, 2018 12:56
@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