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.

Sign up for GitHub

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

Jump to bottom

Added pragmas #3370

Merged
merged 1 commit into from
Apr 11, 2019
Merged

Added pragmas #3370

merged 1 commit into from
Apr 11, 2019

Conversation

sebasguts
Copy link
Member

@sebasguts sebasguts commented Mar 21, 2019 •
edited by fingolfin
Loading

Functions can contain pragmas, i.e., comments
which are stored inside the "compiled" function
as a string.

This is a reimplementation of #1811

Closes #1811

@wucas wucas added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring labels Mar 22, 2019
@codecov
Copy link

codecov bot commented Mar 22, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@54f718c). Click here to learn what that means.
The diff coverage is 74.46%.

@@            Coverage Diff            @@
##             master    #3370   +/-   ##
=========================================
  Coverage          ?   61.77%           
=========================================
  Files             ?      634           
  Lines             ?   306059           
  Branches          ?        0           
=========================================
  Hits              ?   189069           
  Misses            ?   116990           
  Partials          ?        0
Impacted Files Coverage Δ
src/code.h 86.11% <ø> (ø)
src/scanner.h 33.33% <ø> (ø)
src/syntaxtree.c 11.11% <0%> (ø)
src/io.c 46.61% <100%> (ø)
src/code.c 90.56% <100%> (ø)
src/stats.c 63.55% <28.57%> (ø)
src/intrprtr.c 70.28% <83.33%> (ø)
src/read.c 82% <88.88%> (ø)
src/scanner.c 66.51% <93.33%> (ø)

fingolfin
fingolfin reviewed Mar 25, 2019
View reviewed changes
src/intrprtr.c Show resolved Hide resolved
src/read.c Show resolved Hide resolved
src/read.c Show resolved Hide resolved
src/stats.c Outdated Show resolved Hide resolved
src/stats.c Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

So, why is this crashing? Did you investigate already?

@sebasguts
Copy link
Member Author

Sorry, no. Will investigate!

@sebasguts
Copy link
Member Author

So, I would guess the tests fail because of missing pragma support in syntaxtrees. Will add some :)

@sebasguts sebasguts force-pushed the sg/pragmas branch 2 times, most recently from 2b1c836 to 79f5dcc Compare March 27, 2019 00:25
@coveralls
Copy link

coveralls commented Mar 27, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.0004%) to 85.209% when pulling 58c53a9 on sebasguts:sg/pragmas into 54f718c on gap-system:master.

@sebasguts
Copy link
Member Author

@fingolfin Tests pass now.

@ChrisJefferson
Copy link
Contributor

Is there a possible problem with me using #@ as the special marker in Test files?

@sebasguts
Copy link
Member Author

@ChrisJefferson No.
Outside of function coding, the pragmas are simply ignored, so it should not have any impact on the test files.

@olexandr-konovalov
Copy link
Member

Is documentation coming?

@sebasguts
Copy link
Member Author

@alex-konovalov I am not sure where to put the documentation to be honest. I think the best way is to add a new section in the Functions chapter. I will provide add that to the PR

@sebasguts
Copy link
Member Author

@alex-konovalov I updated the PR to include documentation

fingolfin
fingolfin reviewed Apr 11, 2019
View reviewed changes
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.

@sebasguts Oops, just noticed that I forgot to submit these comments yesterday -- sorry for the delay :/

doc/ref/function.xml Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
doc/ref/function.xml Outdated Show resolved Hide resolved
doc/ref/function.xml Outdated Show resolved Hide resolved
@sebasguts sebasguts force-pushed the sg/pragmas branch 2 times, most recently from ed042f1 to 5e10709 Compare April 11, 2019 09:41
fingolfin
fingolfin reviewed Apr 11, 2019
View reviewed changes
doc/ref/function.xml Outdated Show resolved Hide resolved
fingolfin
fingolfin reviewed Apr 11, 2019
View reviewed changes
doc/ref/function.xml Outdated Show resolved Hide resolved
fingolfin
fingolfin reviewed Apr 11, 2019
View reviewed changes
src/stats.c Outdated Show resolved Hide resolved
fingolfin
fingolfin reviewed Apr 11, 2019
View reviewed changes
src/stats.c Outdated Show resolved Hide resolved
@sebasguts
Added pragmas
58c53a9 
Functions can contain pragmas, i.e., comments
which are stored inside the "compiled" function
as a string
@sebasguts sebasguts mentioned this pull request Apr 11, 2019
Merged
wilfwilson
wilfwilson approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

I only feel qualified to review the documentation and tests; both of which I'm happy with. The code itself looks reasonable: there's nothing I can see that looks completely crazy.

@wilfwilson
Copy link
Member

Shall we close #1811 upon merging this PR?

@sebasguts
Copy link
Member Author

Shall we close #1811 upon merging this PR?

I would say so, yes.

@fingolfin fingolfin merged commit ccdd16b into gap-system:master Apr 11, 2019
@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Apr 14, 2019 •
edited
Loading

This now breaks FR package by @laurentbartholdi which has lines with comments looking like

#%%%%%%%%%%%%%%%%%%%%	 RepresentativeActionOp %%%%%%%%%%%%%%%%%%%#

see gap-packages/fr#33

@olexandr-konovalov olexandr-konovalov mentioned this pull request Apr 14, 2019
Closed
@fingolfin
Copy link
Member

What makes you think that this change here causes the breakage in FR? Nothing in the error message there looks like that to me.

@fingolfin
Copy link
Member

Ah OK, in the FR issue you explain it! Still weird; anyway, @sebasguts and me will look into it ASAP.

@olexandr-konovalov
Copy link
Member

@fingolfin you have a point: the first failing build for FR was 459.31 one day earlier than 460.31. They not exactly the same diffs. So there must be something else involved as well.

@sebasguts
Copy link
Member Author

There was actually a bug in the pragma scanning, #3410 should fix it.

@markusbaumeister markusbaumeister added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@DominikBernhardt DominikBernhardt 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 Aug 21, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilfwilson wilfwilson wilfwilson approved these changes

@fingolfin fingolfin fingolfin approved these changes

@markuspf markuspf Awaiting requested review from markuspf

@ChrisJefferson ChrisJefferson Awaiting requested review from ChrisJefferson

Assignees
No one assigned
Labels
gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring 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 topic: kernel
Projects
None yet
Milestone
GAP 4.11.0
Development

Successfully merging this pull request may close these issues.
9 participants
@sebasguts @fingolfin @coveralls @ChrisJefferson @olexandr-konovalov @wilfwilson @DominikBernhardt @wucas @markusbaumeister