Skip to content

Conversation

@fingolfin
Copy link
Member

In GAP one can use Reread (and its variants) instead of Read (and its variants) while developing code; this suppresses errors caused by re-defining global read-only variables.

But this didn't work when using ArithmeticElementCreator because that had its own check for detecting and rejecting such redefinitions.

This patch teaches it to honor the REREADING global for a smoother developer experience.

Based on questions by @TWiedemann

In GAP one can use `Reread` (and its variants) instead of `Read`
(and its variants) while developing code; this suppresses errors
caused by re-defining global read-only variables.

But this didn't work when using `ArithmeticElementCreator` because
that had its own check for detecting and rejecting such
redefinitions.

This patch teaches it to honor the `REREADING` global for a
smoother developer experience.
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Mar 14, 2025
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.

The proposed change addresses the situation where a file gets read with Reread that contains a call to ArithmeticElementCreator.

With this change the following happens.

  • Read the file for the first time. Let us assume the return value of ArithmeticElementCreator is assigned to the variable aec afterwards.
  • Create some arithmetic objects: a:= aec( ... ); b:= aec( ... ); ....
  • Reread the file. Now aec is a new object.
  • Create more arithmetic objects: c:= aec( ... ); ....
  • Now a + b works, but a + c does not work because a and c have been created by different creator objects.

In this sense, isn't this change asking for trouble?

Of course one can argue that the same happens if a file contains the creation of a free group, and rereading this file will create a new group whose elements are not compatible with elements of the original group.
However, I think the difference is that here one can see that the underlying objects are different, whereas the return value of ArithmeticElementCreator looks like an innocent function, and rereading such code is regarded as safe.

@fingolfin
Copy link
Member Author

While all your points are valid, I still think this change is beneficial: in the end, Reread is a very sharp tool; even without ArithmeticElementCreator involved it can cause severe trouble, and the problems you list can also appear there.

In my eyes it is a low-level tool for developers who (think they) know what they are doing. I see no advantage in defeating it in this scenario for the sake of protecting developers from shooting themselves in their foot -- in my mind if we followed that line of reasoning then we should abolish Reread completely. (To be clear, I don't want that ;-) )

@fingolfin
Copy link
Member Author

@ChrisJefferson any thoughts on this?

@ChrisJefferson
Copy link
Contributor

I agre I'm happy with this. I'd also be happy to ban Reread, but if we are going to keep supporting it, this make is function in another case. There are lots of ways Reread can get you in an inconsistent state.

@fingolfin fingolfin merged commit 8c0b08c into gap-system:master May 15, 2025
33 checks passed
@fingolfin fingolfin deleted the mh/proto-rereading branch May 15, 2025 16:31
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: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants