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

Improve tracking of InstallMethod and DeclareOperation #2422

Merged

Conversation

fingolfin
Copy link
Member

So far, we used INPUT_LINENUMBER() to record the location of an InstallMethod or DeclareOperation call. But in case such a call takes multiple lines, that points to the last line, while we would prefer if it pointed to the first line. To achieve that, we add a new global variable READEVALCOMMAND_LINENUMBER which is updated at the start of ReadEvalCommand.

One may debate whether it might be better if READEVALCOMMAND_LINENUMBER was a global function which returns the desired result. If somebody has a good argument for that, I'll be happy to adapt the patch. The only reason I used a global variable is that it was the quickest way to implement this.

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Apr 30, 2018
@codecov
Copy link

codecov bot commented Apr 30, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2422      +/-   ##
==========================================
+ Coverage   73.88%   73.89%   +<.01%     
==========================================
  Files         484      484              
  Lines      245465   245477      +12     
==========================================
+ Hits       181371   181383      +12     
  Misses      64094    64094
Impacted Files Coverage Δ
hpcgap/src/c_oper1.c 89.06% <100%> (+0.02%) ⬆️
lib/oper.g 81.18% <100%> (ø) ⬆️
src/read.c 90% <100%> (ø) ⬆️
src/c_oper1.c 90.76% <100%> (+0.02%) ⬆️
src/hpc/threadapi.c 43.3% <0%> (-0.11%) ⬇️
hpcgap/lib/hpc/stdtasks.g 64.7% <0%> (+0.25%) ⬆️

@ssiccha
Copy link
Contributor

ssiccha commented Apr 30, 2018

I don't have strong feelings about READEVALCOMMAND_LINENUMBER being a gvar or a function.

LGTM

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

I am happy with the solution to access the first line of a method installation.
Since now the first and the last line of a method installation are in principle available, would it make sense to store both values?

@fingolfin fingolfin force-pushed the mh/better-InstallMethod-tracking branch from 6721469 to 12ccbdd Compare May 1, 2018 19:34
@fingolfin
Copy link
Member Author

@ThomasBreuer good point; I changed it to now store a triple [ filename, firstline, lastline ].

@fingolfin fingolfin force-pushed the mh/better-InstallMethod-tracking branch from 12ccbdd to b767782 Compare May 1, 2018 20:11
So far, we used `INPUT_LINENUMBER()` to record the location of an
InstallMethod or DeclareOperation call. But in case such a call takes
multiple lines, that points to the *last* line, while we would prefer if it
pointed to the *first* line. To achieve that, we add a new global variable
`READEVALCOMMAND_LINENUMBER` which is updated at the start of
`ReadEvalCommand`.
@fingolfin fingolfin merged commit b247e55 into gap-system:master May 2, 2018
@fingolfin fingolfin deleted the mh/better-InstallMethod-tracking branch May 2, 2018 14:18
@fingolfin fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Jul 31, 2018
@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 Sep 20, 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.

3 participants