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

Fix code location when reading history from a file #3416

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

d-torrance
Copy link
Member

At startup, we use a new compiled function (historyLength(), which returns the value of readline's history_length variable) to determine how many lines of history we've saved from previous sessions. We add this value to the line numbers of any code from stdio when calling code so we get the correct lines back when calling getHistory.

Before

i1 : f = x -> x^2;

i2 : code f

o2 = stdio:1:6-1:11: --source code:
     -- This is the beginning of your Macaulay2 log stored at /home/profzoom/.Macaulay2/history.m2

After

i1 : f = x -> x^2;

i2 : code f

o2 = stdio:1:4-1:12: --source code:
     f = x -> x^2;

Closes: #3415

Returns value of history_length variable from readline history.
When reading the history from a file, we need to add the number of
lines loaded from previous sessions when calling getHistory to get the
correct lines of code.

Closes: Macaulay2#3415
@@ -143,6 +143,8 @@ char *system_getHistory(const int n)
return NULL;
}

int system_historyLength() { return history_length; }
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to directly export this variable to the top-level?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my first thought, too. But I couldn't figure it out -- if we do something like setupvar("historyLength", toExpr(Ccode(int, "history_length"))), then it will just be 0 since it gets assigned at startup and never updates.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it needs to be a pointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

But we'd still need to dereference it when we need its value, so we'd have to call some kind of function. I suppose we could add some variation of setupvar that does this, but I'm not sure if it's worth it.

@mahrud
Copy link
Member

mahrud commented Aug 20, 2024

Do we have a mechanism to test this? Not sure how exactly to test readline ...

Copy link
Member

@mahrud mahrud left a comment

Choose a reason for hiding this comment

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

Two possible improvements:

  • export history length directly, not sure how easy this is
  • add readline tests to make sure this doesn't break again

M2/Macaulay2/m2/Core.m2 Outdated Show resolved Hide resolved
Copy link
Member

@mahrud mahrud left a comment

Choose a reason for hiding this comment

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

Feel free to merge once tests pass.

@d-torrance d-torrance merged commit 1658a78 into Macaulay2:development Aug 21, 2024
5 checks passed
@d-torrance d-torrance deleted the history branch September 3, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants