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

Make Proof Rule enum a part of the API #9925

Merged
merged 22 commits into from
Sep 27, 2023

Conversation

hansjoergschurr
Copy link
Contributor

This pull requests makes the enum that lists all proof rules a part of the API.

It also renames the enum from PfRule to ProofRule. It also renames some unrelated types and function names that share the PfRule name (such as DslPfRule).
This rename unfortunately touches many files since PfRule is not an uncommon type. (second to last commit)

I also added the enum to the Java and Python API, but I am not sure I got this right. It is in the last commit.

@hansjoergschurr hansjoergschurr added normal Priority moderate Complexity labels Aug 1, 2023
@hansjoergschurr
Copy link
Contributor Author

hansjoergschurr commented Aug 2, 2023

Oh I noticed that I broke something with my last commit. Will investigate.

@hansjoergschurr hansjoergschurr force-pushed the devel/public-rule-type branch 4 times, most recently from b22fd2a to 3d9487e Compare September 7, 2023 20:26
@hansjoergschurr hansjoergschurr added the do not merge Do not merge this PR if you are not the author label Sep 8, 2023
@hansjoergschurr hansjoergschurr changed the title Make Proof Rule enum a part of the API WIP Make Proof Rule enum a part of the API Sep 8, 2023
@ajreynol
Copy link
Member

Is this PR ready to go? I assume this is the first PR to go in for the proof API?

@hansjoergschurr hansjoergschurr removed the do not merge Do not merge this PR if you are not the author label Sep 12, 2023
@hansjoergschurr hansjoergschurr changed the title WIP Make Proof Rule enum a part of the API Make Proof Rule enum a part of the API Sep 12, 2023
@hansjoergschurr
Copy link
Contributor Author

If it passes the CI it should be ready to go. I just added Mathjax support.

I am not 100% happy with including the config in the Javadoc CMake file. It seems fragile and somewhat repetitious, but I am not sure if there is a much better place.

docs/api/java/CMakeLists.txt Show resolved Hide resolved
include/cvc5/cvc5_proof_rule.h Outdated Show resolved Hide resolved
Copy link
Member

@ajreynol ajreynol left a comment

Choose a reason for hiding this comment

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

A few requests. Also, I believe you will need to add the print function for ProofRule to the unit tests here:
https://github.com/cvc5/cvc5/blob/main/test/unit/api/python/test_uncovered.cpp
https://github.com/cvc5/cvc5/blob/main/test/unit/api/java/UncoveredTest.cpp

Also a cpp unit test for printing proof rules needs to be added I think, similar to:
https://github.com/cvc5/cvc5/blob/main/test/unit/api/cpp/api_kind_black.cpp

include/cvc5/cvc5_proof_rule.h Outdated Show resolved Hide resolved
include/cvc5/cvc5_proof_rule.h Outdated Show resolved Hide resolved
This moves the header file that contains the proof rule enum
to the public headers and also moves the associated cpp file.

Furthermore, it removes the `internal` namespace from those files.
This also renames some internal funtions and types that contain
PfRule.  This ensures consistency.
hansjoergschurr and others added 14 commits September 26, 2023 16:51
This uses a Regex to first split the comment at the two types of
math tags and then skips thoes during replacements.
The old code to translate RST lists was very specific to the types
of list that occured the enum docs.  The new one should be more
general.  It can handle abritrarily nested lists.
The translator sees closing parentheses as part of a funtion
refrerence and would incorrectly move them inside an `{@ link ..}`
@hansjoergschurr
Copy link
Contributor Author

I think I added the appropriate unit tests.

@ajreynol ajreynol enabled auto-merge (squash) September 27, 2023 00:18
@ajreynol ajreynol merged commit 0a35879 into cvc5:main Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
moderate Complexity normal Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants