Make ValueGlobal faster

[sebasguts]

... by turning ValueGlobal into a synonym of VALUE_GLOBAL.

The check if the argument of ValueGlobal is a string is done by VALUE_GLOBAL, too, so this check is not necessary.

The fact if all characters in the argument string are in IdentifierLetters does not cause an error, just an info. Furthermore, variables with different characters in their identifier, e.g., \ , can still be written to and accessed.

[ChrisJefferson]

Looks like @stevelinton may have written this file originally.

My main comment is that I don't really like just doing this to ValueGlobal, when there are many other functions in the same file (IsBoundGlobal, MakeReadOnlyGlobal,...), which all have a similar design to ValueGlobal. I would prefer we simplify all of them, or none of them, unless there is a good reason to just change one.

[stevelinton]

That was me. A very long time ago.

I don't really like the use of DeclareSynonym here. Better to simply install VALUE_GLOBAL as the value of the global function -- performancewise it should make no difference, but it seems nicer to read that way, and (since all the declared functions now have some kind of implementation in the .gi file, makes it less urgent to adjust the other functions that @ChrisJefferson suggests.

[codecov]

Codecov Report

Merging #2232 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
- Coverage   69.88%   69.88%   -0.01%     
==========================================
  Files         481      481              
  Lines      252990   252986       -4     
==========================================
- Hits       176814   176805       -9     
- Misses      76176    76181       +5

Impacted Files	Coverage Δ
lib/global.gi	40.14% <ø> (-1.7%)	⬇️
hpcgap/lib/hpc/queue.g	66.4% <0%> (-3.2%)	⬇️
src/hpc/threadapi.c	36.9% <0%> (-0.1%)	⬇️

[fingolfin]

I agree with @stevelinton : I'd leave this declaration here, and then just change the .gi file to InstallGlobalFunction( ValueGlobal, VALUE_GLOBAL ), simply for symmetry with the rest.

The difference is in the function names:

gap> DeclareSynonym("foo", VALUE_GLOBAL);
gap> DeclareGlobalFunction("bar");
gap> InstallGlobalFunction( bar, VALUE_GLOBAL );
gap> NameFunction(foo);
"VAL_GVAR"
gap> NameFunction(bar);
"bar"

Which reminds me of another thing: VALUE_GLOBAL itself is just an alias for VALUE_GVAR, and it is not even write protected: You can e.g. do VALUE_GLOBAL := 1...

So we may want to unify that, too. But of course we have to be careful because there are packages using almost any of the three names in provided for each of the functions dealing with globals sigh. (Strictly speaking, packages using the undocumented functions VALUE_GLOBAL, VALUE_GVAR etc are doing something wrong, and we are not responsible for keeping them working, but, well, let's be realistic, people will still yell at us, not the package authors, if we change this without either providing backward compatibility code, or else getting all packages using it fixed first....

[sebasguts]

I adjusted the commit and the PR to @stevelinton 's suggestions. I also agree completely to @ChrisJefferson 's suggestions, by making that change to many of the functions in global.gd. But I think this could be done in a different PR.

[fingolfin]

@sebasguts thanks, but now the commit message does not fit :-).

I think the commit message should mention that calling CheckGlobalName really is not necessary, because if the name was not valid internally, it'll just do nothing.

BTW, while I won't miss the Info call there or anywhere else, I wonder if others might? I have a hard time seeing what they would be useful for, ever, but that just shows my lack of imagination. Perhaps @stevelinton can give some insights?

Anyway, if people were to miss it, I wonder if just removing the call to CheckGlobalName resolves your problems?

Final remark: While I do not expect you to make other changes, I'd like to point out that most other CheckGlobalName calls could be renamed with the same argument; I think really only the ones in BindGlobal and BindConstant matter.

[sebasguts]

@fingolfin Why does the message not fit? It is still a synonym, just not in the technical sense. But I can change it if you like. Then I can also point out why the call is not necessary.

I also generally agree that the call for CheckGlobalName is not necessary in almost all cases. I think even remove it for BindGlobal since the fact if the input is a string is still checked by the kernel function, and the checked letters are just documented as "the usual characters in an identifier".

[fingolfin]

OK, I guess the commit message is fine after all.

[fingolfin]

I have approved this PR, but I'd like @ChrisJefferson and/or @stevelinton to decide its ultimate fate for now.