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

Solver policies should be global constants (like C enums) #584

Closed
pgoodman opened this issue Nov 21, 2017 · 7 comments
Closed

Solver policies should be global constants (like C enums) #584

pgoodman opened this issue Nov 21, 2017 · 7 comments

Comments

@pgoodman
Copy link
Member

Summary of the problem

Right now manticore uses strings to represent solver policies. For example, there is MINMAX, etc. These are encoded in the Concretize._ValidPolicies list. These should really be named constants with numeric values, and they should be more uniformly used.

For example, the solve_n method of the State class has minmax as its policy, and it's not clear if that default policy is even possible or meaningful.

    def solve_n(self, expr, nsolves, policy='minmax'):
@feliam
Copy link
Contributor

feliam commented Nov 21, 2017

The api description of solve_n should describe all its preconditions/valid inputs.
Ideally the user facing string representation of the policies should be defined in a single place.
Numeric constants do not provide any improvement over constant strings. String constants are more human readable. You can but you do not really need to import any module to use them by name.
Internally a string constant is a number.

I do not mind if there is a module somewhere with a bunch of constants defined

ALL_POLICY = "ALL"
MINMAX_POLICY = "MINMAX"

Full disclosure, I'll use "MINMAX" instead of from modulex import MINMAX_POLICY; MINMAX_POLICY

@offlinemark
Copy link
Contributor

I agree that we should have a module with constants for policies, and strictly use only it. I think it's less important whether to use strings to ints, but strings make it more tempting to not use the constants.

regardless, @pgoodman raised an excellent point that solve_n with minmax doesn't make sense. honestly, those State solve APIs need a total rework, the existence of both solve_one, and solve_n is poor design and was a big mistake on my part (i made them originally)

@feliam
Copy link
Contributor

feliam commented Nov 21, 2017

Let's do not get strict. I vote not to force users to import stuff just to do a small script.
Slow but forgiving is better than slow and strict.
I normally use solver.get_all_values or solver.get_value.

@offlinemark
Copy link
Contributor

could we make the constants available through class variables, so no import is needed?

class Manticore:
  SOLVER_MINMAX = qwer
  SOLVER_ALL = zxcv
  SOLVER_SAMPLED = asdf

then users could easily use them without import, with tab-complete in their editor/ide, with equal readability to strings.

the downside is that the Manticore class is not the right place for those constants.

@feliam
Copy link
Contributor

feliam commented Nov 21, 2017

    class Policy():
        ALL = 'ALL'
        ONE = 'ONE'
        MINMAX = 'MINMAX'

        @staticmethod
        def check(x):
            d = Policy.__dict__
            return x in [name for name in d.keys() if d[name] == name]
                

    print Policy.check('ALL')
    print Policy.check(Policy.ALL)

@ehennenfent ehennenfent added this to the Validate Existing issues milestone Jan 23, 2019
@ehennenfent ehennenfent removed this from the Validate Existing issues milestone Feb 26, 2019
@ehennenfent
Copy link
Contributor

@pgoodman
Copy link
Member Author

In retrospect, I think policies should actually be functions or classes, not enums.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants