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: reject invalid AND-filters #2756

Merged

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Aug 28, 2018

This rejects invalid AND-filters like this:

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

This contains PR #2755 and thus further extends the work begun in PR PR #2732. However, it should not be merged until fixed versions of packages relying on the old buggy behavior are part of the distribution. Specifically, these packages (they all already merged a fix):

  • MatricesForHomalg
  • qpa
  • xmodalg

Alternatively, we could skip merging PR #2755, and directly merge a (squashed) version of this PR. That depends a bit on whether we decide to merge PR #2755 for GAP 4.10 or not, and/or on how quick fixed releases of these packages appear.

@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
@codecov
Copy link

codecov bot commented Sep 12, 2018

Codecov Report

Merging #2756 into master will decrease coverage by <.01%.
The diff coverage is 39.47%.

@@            Coverage Diff             @@
##           master    #2756      +/-   ##
==========================================
- Coverage   83.77%   83.77%   -0.01%     
==========================================
  Files         681      681              
  Lines      346620   346651      +31     
==========================================
+ Hits       290368   290392      +24     
- Misses      56252    56259       +7
Impacted Files Coverage Δ
src/opers.h 100% <ø> (ø) ⬆️
src/error.c 77.77% <0%> (-1.9%) ⬇️
src/exprs.c 97.59% <100%> (+1.01%) ⬆️
src/compiler.c 88.37% <100%> (+0.1%) ⬆️
src/intrprtr.c 98.83% <100%> (-0.01%) ⬇️
src/c_oper1.c 93.55% <11.11%> (-0.15%) ⬇️
src/hpc/c_oper1.c 92.74% <16.66%> (-0.05%) ⬇️
src/c_type1.c 84.85% <33.33%> (-0.09%) ⬇️
src/hpc/c_type1.c 85.08% <50%> (-0.05%) ⬇️
src/opers.c 94.8% <75%> (-0.05%) ⬇️
... and 5 more

@olexandr-konovalov
Copy link
Member

I cross-referenced this in #2804 under "PRs blocked by packages".

@sunnyquiver
Copy link

The QPA-team has now created a new release 1.28 incorporating the fix.

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Oct 9, 2018

QPS: @sunnyquiver thanks, I see gap-packages/qpa#16 merged. There was another problem with the new release of QPA, so waiting now for the next one.

XModAlg: ticked the box, gap-packages/xmodalg#25 is now in version 1.17 from 30/08/2018.

homalg: ticked the box for homalg-project/homalg_project#205 merged in MatricesForHomalg and released in version 2018.08.25 dated 25/08/2018.

@olexandr-konovalov
Copy link
Member

@fingolfin all three packages are now in bootstrapping package archives - please rebase, and I hope we will be able to merge this after the test (hope no other packages did this in the meantime).

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

Looks good to me. Checked package loading log https://travis-ci.org/gap-system/gap/jobs/440558613 - this PR does not trigger any errors when packages are loaded.

Will this consist of three commits, or two of them will be glued together?

@fingolfin
Copy link
Member Author

I'll squash them, and also look into fixing the compiler variant.

@fingolfin fingolfin force-pushed the mh/even-stricter-and-filter branch 2 times, most recently from ec4fa8a to 6d3ba85 Compare October 13, 2018 14:19
@fingolfin fingolfin changed the title kernel: reject invalid AND-filters (DO NOT MERGE) kernel: reject invalid AND-filters Oct 13, 2018
Also unify treatment of 'and' for filters in interpreter and executor.

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 reject such code.
@olexandr-konovalov olexandr-konovalov merged commit 3399870 into gap-system:master Oct 17, 2018
@fingolfin fingolfin deleted the mh/even-stricter-and-filter branch October 17, 2018 16:19
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 15, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants