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

Read lib/transatl.g before reading gap.ini #3010

Merged
merged 1 commit into from
Nov 17, 2018

Conversation

wilfwilson
Copy link
Member

I have made this PR as a result of what I think would be the best solution to #2828 and #2996.

Currently, lib/transatl.g is read in init.g a bit later than gap.ini. This means that if a package is autoloaded from gap.ini and the package uses a name declared in lib/transatl.g, then an unbound global variable warning is given. This happens with GRAPE 4.8.1 in particular.

This PR moves the reading of lib/transatl.g to the point directly before reading gap.ini.

Anyway, I've created this PR as a discussion point. There might be very good reasons for why this is a bad idea; in this case, I will be keen to hear about them!

Currently, if a package is autoloaded from `gap.ini` and uses
a name declared in `lib/transatl.g`, then an `unbound global
variable` warning is given.

Closes gap-system#2828.
Closes gap-system#2996.
@fingolfin
Copy link
Member

Can you please edit the issue message to say Resolve #XYZ for all (both?) issues that would be resolved by merging this? Thank you!

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.

To me this seems very sensible and plausible.

@fingolfin
Copy link
Member

AFAICT, @hulpke originally wrote this code, so he might have additional insights. And perhaps some of the other people who've been around longer, such as @stevelinton @frankluebeck @ThomasBreuer ?

@wilfwilson
Copy link
Member Author

wilfwilson commented Nov 16, 2018

@fingolfin Ah, sorry, I used Closes rather than Resolve – now I think about it, I suppose this closes the issue automatically without resolving it.

@wilfwilson
Copy link
Member Author

Oh wait, no, there is no separate notion of closed or resolved. Is it okay how it is @fingolfin?

@olexandr-konovalov
Copy link
Member

The consideration could be that e.g. before calling Transatlantic(IsPSolvable) one should have IsPSolvable declared already. Reading it earlier like in this PR does not break this. So it looks good for me then.

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

Originally GAP had the convention that US spelling was used.

This bothered some users, who felt that if Stabiliser was good enough for her Majesty, it should be good enough for GAP. transatl.g simply does this renaming in bulk, rather than add a synonym in each declaration. As it only does so for library declared operations, it is fine to have it be used before reading in gap.ini.

The only reason I can see for loading it after gap.ini would be to enforce that packages should use the declared name and not synonyms. I can't see a good reason for doing so, and it would be a fight against windmills in any case.

@fingolfin
Copy link
Member

@wilfwilson it is OK as it is -- I didn't see that you have the Closes lines in the commit messages, only looked at the PR description (where I usually put them, to make them more visible -- this is not a hidden request to change that here, BTW, just an explanation for my thought process :)

@ThomasBreuer
Copy link
Contributor

ThomasBreuer commented Nov 16, 2018

I have no strong preferences about this topic, but since I have been asked:

  • An argument for not reading transatl.g at all would be that due to the synonyms,
    tab completion stops before the end of the name even if the function is unique.

  • An argument for reading transatl.g as late as possible would be
    that then it would be easier to find occurrences of function names with grep.

  • The selection of function names for which transatlantic variants are available looks quite arbitrary.
    For example attribute testers and setters are not handled.
    The fact that this has not been observed indicates that the feature is used rarely.
    (The code would be able to deal with DeclareSynonymAttr, this is just not used.
    And of course one could simplify the code.)

@lhsoicher
Copy link

Thanks @wilfwilson. I agree with you. That said, I am happy to change any "Stabiliser" to "Stabilizer" for the next release of GRAPE.

@ChrisJefferson ChrisJefferson merged commit 33d499f into gap-system:master Nov 17, 2018
@wilfwilson wilfwilson deleted the read-transatl-earlier branch November 24, 2018 21:48
@PaulaHaehndel PaulaHaehndel 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
@wilfwilson wilfwilson 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 Apr 24, 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
Labels
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.

None yet

8 participants