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

Added array literal creation to API and implementation #367

Closed
wants to merge 4 commits into from

Conversation

leventeBajczi
Copy link
Contributor

As mentioned in #360, currently, array literals are not supported by the API. This PR adds two new methods to this end, one without a default value, and one with a default (else) value. I also added some tests to verify that these work normally.

long func =
getFormulaCreator()
.declareUFImpl(
"__unnamed_arr", toSolverType(pElementType), List.of(toSolverType(pIndexType)));
Copy link
Member

@kfriedberger kfriedberger Mar 24, 2024

Choose a reason for hiding this comment

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

Is this extra UF really necessary?
How can we create two arrays with default values, when they have distinct types? (test missing)
Is this name forbidden as normal variable name and excluded in model generation? (test missing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see another way of creating a non-initialized array literal. I added some extra tests to verify that these do not create problems with e.g. vars with the same name. It turns out that only Z3 works well with such literals (without a default value), and only Z3, MathSAT and CVC5 work with initialized literals.

@leventeBajczi
Copy link
Contributor Author

I can see that the SolverConcurrencyTest seems to be failing. However, I cannot reproduce the problem locally. Can you please take a look?

@baierd
Copy link
Collaborator

baierd commented Mar 24, 2024

SolverConcurrencyTest is flaky. If its not reproducable locally or continuously in the CI you may ignore that test class.

Copy link
Member

Choose a reason for hiding this comment

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

Please do not change basic project files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry, I didn't pay attention when adding files to the commit (and IntelliJ is adamant on changing these all the time..)

FormulaType.getArrayType(pIndexType, pElementType);
final Sort cvc5ArrayType = toSolverType(arrayFormulaType);
return solver.mkConstArray(cvc5ArrayType, elseElem);
}
Copy link
Member

Choose a reason for hiding this comment

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

I took this part of the code and merged it to master.

FormulaType.getArrayType(pIndexType, pElementType);
final Long mathsatArrayType = toSolverType(arrayFormulaType);
return msat_make_array_const(mathsatEnv, mathsatArrayType, elseElem);
}
Copy link
Member

Choose a reason for hiding this comment

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

I took this part of the code and merged it to master.

protected <TI extends Formula, TE extends Formula> Long internalMakeArray(
Long elseElem, FormulaType<TI> pIndexType, FormulaType<TE> pElementType) {
return Native.mkConstArray(z3context, toSolverType(pIndexType), elseElem);
}
Copy link
Member

Choose a reason for hiding this comment

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

I took this part of the code and merged it to master.

// init array cannot read any value besides default
assertThatFormula(bvmgr.equal(numM1, amgr.select(arr, num2))).isUnsatisfiable();
assertThatFormula(bvmgr.equal(num0, amgr.select(arr, num2))).isTautological();
}
Copy link
Member

Choose a reason for hiding this comment

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

I took this part of these tests as basis for further tests that are merged to master.

@leventeBajczi
Copy link
Contributor Author

I believe you're right that uninitialized arrays do not make a lot of sense. I also see that for the initialized arrays you merged the modifications to master manually - I'm closing this PR. Thanks for the help!

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

Successfully merging this pull request may close these issues.

3 participants