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

Avoid using EvalString #463

Closed
fingolfin opened this issue Feb 23, 2018 · 2 comments
Closed

Avoid using EvalString #463

fingolfin opened this issue Feb 23, 2018 · 2 comments
Assignees
Labels
minor A label for issues or PRs that are not major.

Comments

@fingolfin
Copy link
Contributor

I just noticed that Semigroups (ab)uses EvalString quite a bit (I found 51 hits with grep). This is a function I'd recommend to never use, except for interactive debugging.

One place it is used in Semigroups is this:

for _IsXSemigroup in ["IsFpSemigroup",
                     "IsFpMonoid",
                     "IsNTPMatrixSemigroup",
                     "IsMaxPlusMatrixSemigroup",
                     "IsMinPlusMatrixSemigroup",
                     "IsTropicalMaxPlusMatrixSemigroup",
                     "IsTropicalMinPlusMatrixSemigroup",
                     "IsProjectiveMaxPlusMatrixSemigroup",
                     "IsIntegerMatrixSemigroup",
                     "IsReesMatrixSemigroup",
                     "IsReesZeroMatrixSemigroup"] do
  InstallMethod(TrivialSemigroupCons,
  Concatenation("for ", _IsXSemigroup, " and an integer"),
  [EvalString(_IsXSemigroup), IsInt],
  function(filter, deg)
    local n;
    n := Maximum(deg, 1);
    return AsSemigroup(filter,
                       TrivialSemigroupCons(IsTransformationSemigroup, deg));
  end);
od;

Here, a much better replacement is to use ValueGlobal(_IsXSemigroup). It should even be faster.

Another case is gap/tools/utils.gi, which does EvalString(Concatenation("IsBound(", name, ")")) . I suspect that there, you can get aways with using IsBoundGlobal(name) instead.

This leaves the tests, where your use of EvalString seems OK.

@james-d-mitchell
Copy link
Collaborator

Thanks @fingolfin will fix this as soon as I am able.

@james-d-mitchell james-d-mitchell self-assigned this Mar 19, 2018
@james-d-mitchell james-d-mitchell added minor A label for issues or PRs that are not major. 3.0 labels Mar 19, 2018
james-d-mitchell pushed a commit to james-d-mitchell/Semigroups that referenced this issue Mar 21, 2018
james-d-mitchell added a commit that referenced this issue Mar 21, 2018
Resolve Issue #463: EvalString -> ValueGlobal
@james-d-mitchell
Copy link
Collaborator

This is resolved via PR #464. Thanks for pointing this out @fingolfin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor A label for issues or PRs that are not major.
Projects
None yet
Development

No branches or pull requests

2 participants