Add DeclareGlobalName; together with BindGlobal it can be used to replace uses of DeclareGlobalVariable & InstallValue (which are not thread safe and have other dangers)

[fingolfin]

With DeclareGlobalName one can mark the name of a global variable as "will be defined". The only effect this has is that it suppresses "Unbound Global Variable" warnings.

This can be used in many cases to replace uses of DeclareGlobalVariable & InstallValue respectively DeclareGlobalFunction & InstallGlobalFunction by DeclareGlobalName & BindGlobal.

This PR complements PR #3857.

TODO:

• add documentation for DeclareGlobalName (didn't want to spend work on that before I had any feedback on this design)
• bikeshed^H^H^ decide the name: DeclareGlobalName? DeclareName? DeclareGlobal? Something else?

[ChrisJefferson]

So, my only thought is that unlike the old DeclareGlobalVariable / InstallName, we don't "check" the two are in sync, so I can do DeclareGlobalName("nameA");, then BindGlobal("nameB", [1,2,3]), and nothing there is no warning I've used "nameA" in one place, and "nameB" in another.

This can't be fixed without introducing something like BindGlobalName (which is definitely a horrible name) which will only bind things previously defined with DeclareGlobalName.

We might decide this isn't a significant concern, but I just wanted to mention it.

[fingolfin]

If that happens, then one of these happens:

• if any code tries to use nameA, it will immediately run into an error, and the issue is discovered;
• otherwise, if only nameB is used and no "unbound global variable" is shown, then the call to DeclareGlobalName was redundant and can be removed. Of course nobody might notice -- but there seems no harm in this, other than perhaps confusing somebody (e.g. because nameA appears in the documentation) -- but once they try to use it, they'll immediately get an error (and hopefully report it to us / to the author of the code in question).

So all in all, it seems there is not much we loose there?

[coveralls]

Coverage decreased (-0.0008%) to 84.33% when pulling 5eabdf1 on fingolfin:mh/DeclareGlobalName into 8e10d8a on gap-system:master.

[ChrisJefferson]

I'm happy with this. I'm going to give it another few days for anyone who wants to comment on design / names.

[fingolfin]

But this still lacks documentation...

[wilfwilson]

I think the setup is fine. I’d perhaps prefer just DeclareGlobal, but as you say - bikeshedding, and DeclareGlobalName is totally clear and appropriate.

Would approve with doc.

[ssiccha]

I like the idea. 👍

[fingolfin]

Note that the second argument to DeclareGlobalVariable always was ignored, so it's safe to discard it here and elsewhere.

[fingolfin]

I've now added some documentation, so this could potentially be merged, after a fresh round of reviews.

[fingolfin]

Note that I only added docs for DeclareGlobalName, but I did not adjust the docs for DeclareGlobalVariable and DeclareGlobalFunction to reference it; I guess I should, though, and explain there that DeclareGlobalName usually is preferable? Argh. Anybody here like writing docs more than I do? sigh

[ssiccha]

I've updated and slightly tweaked the documentation of DeclareGlobalVariable and DeclareGlobalFunction and I've fixed typos in the doc of DeclareGlobalName. What do all of you think?

[ChrisJefferson]

one tiny comment

[fingolfin]

@ssiccha thanks! I've now squashed this, split the commit differently, and edited the documentation further. Please have a look again.

[fingolfin]

I split the actual changes which replace uses of DeclareGlobalVariable by DeclareGlobalName into a second commit, so that it's easier tor review the core changes by just looking at the first commit.

I should note that one could (should?) also treat most uses of DeclareGlobalFunction this way, also it is less important, because we "only" modify function objects here, and that's a bit less critical... although in the HPC-GAP context it could still be serious problem. I just didn't have time for it and it wasn't a priority, but a future PR could take care of many of them.

And once this release in GAP 4.12, we could of course start changing packages to use this, but I expect this to be a slow process

[ssiccha]

LGTM, apart from the typo.

[fingolfin]

@ChrisJefferson OK to approve/merge?

[ChrisJefferson]

Yes, I'm happy. Thanks all!