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

Omit tester filters from ShowImpliedFilters output #2243

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

ThomasBreuer
Copy link
Contributor

This is a follow-up to #2224. As discussed there,
those property tester filters are omitted from the output
for which the property itself is already shown.

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.

Nice improvement! Just two minor remarks.

lib/methwhy.g Outdated
@@ -393,7 +394,7 @@ BIND_GLOBAL("PageSource", function ( fun, nr... )
f := Filename(List(GAPInfo.RootPaths, Directory), f);
fi;
if f = fail and fun in OPERATIONS then
# for operations we show the location(s) of their operation
# for operations we show the location(s) of their declararion
Copy link
Member

Choose a reason for hiding this comment

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

Typo, r instead of t

lib/methwhy.g Outdated
@@ -322,20 +319,24 @@ local flags, implied, f, extra_implications, implication, name, diff_reqs, diff_
implied := SUB_FLAGS(implied, flags);
fi;

reduced:= trues -> Filtered( trues,
i -> not ( INFO_FILTERS[i] in FNUM_TPRS
and FLAG1_FILTER( FILTERS[i] ) in trues ) );
Copy link
Member

Choose a reason for hiding this comment

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

For HPC-GAP, this requires atomic readonly FILTER_REGION do ... od around the access to INFO_FILTERS and FILTERS, I am afraid. So (modulo typos):

reduced := functon(trues)
  atomic readonly FILTER_REGION do
    return not ( INFO_FILTERS[i] in FNUM_TPRS
                and FLAG1_FILTER( FILTERS[i] ) in trues ) );
  od;
end;

@codecov
Copy link

codecov bot commented Mar 5, 2018

Codecov Report

Merging #2243 into master will increase coverage by <.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #2243      +/-   ##
==========================================
+ Coverage   70.57%   70.57%   +<.01%     
==========================================
  Files         481      481              
  Lines      253237   253244       +7     
==========================================
+ Hits       178728   178735       +7     
  Misses      74509    74509
Impacted Files Coverage Δ
lib/methwhy.g 49.56% <88.88%> (+1.14%) ⬆️
src/hpc/traverse.c 94.97% <0%> (-0.51%) ⬇️
src/hpc/threadapi.c 36.9% <0%> (+0.18%) ⬆️

This is a follow-up to gap-system#2224.  As discussed there,
those property tester filters are omitted from the output
for which the property itself is already shown.
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.

Thanks, LGTM!

Off-topic: I just now noticed this PR had been updated, by pure chance -- unfortunately, when new code is pushed into a PR, GitHub does not notify reviewers. So I suggest to add a brief comment, like "updated this PR", so that reviewers may notice the change and update their reviews.

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 20, 2018
@fingolfin fingolfin merged commit fdd12d6 into gap-system:master Mar 20, 2018
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements 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.

2 participants