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

Delete functions instead of using CVC4_UNDEFINED #1794

Merged
merged 7 commits into from
Aug 8, 2018

Conversation

4tXJ7f
Copy link
Member

@4tXJ7f 4tXJ7f commented Apr 20, 2018

C++11 supports explicitly deleting functions that should not be used
(explictly or implictly), e.g. copy or assignment constructors. We were
previously using the CVC4_UNDEFINED macro that used a compiler-specific
attribute. The C++11 feature should be more portable.

@timothy-king: I'm adding you as a reviewer because this could be of interest to you.

C++11 supports explicitly deleting functions that should not be used
(explictly or implictly), e.g. copy or assignment constructors. We were
previously using the CVC4_UNDEFINED macro that used a compiler-specific
attribute. The C++11 feature should be more portable.
@4tXJ7f 4tXJ7f added minor Priority simple Complexity labels Apr 20, 2018
@4tXJ7f
Copy link
Member Author

4tXJ7f commented Apr 20, 2018

Great, I guess we have another reason to switch to SWIG 3 (it looks like SWIG 2 has issues with = delete)... :(

@timothy-king
Copy link
Contributor

You have already brought up SWIG. It is my biggest concern with making this type of change. Please make sure to additionally test building the java examples as well as building from scratch before checking this in.

@4tXJ7f 4tXJ7f added the do not merge Do not merge this PR if you are not the author label May 5, 2018
@4tXJ7f
Copy link
Member Author

4tXJ7f commented Aug 5, 2018

Alright, so it turns out that CVC4 builds with Swig 3. I am not sure what happened to the earlier issues that we were seeing (#1535). After updating Travis to use Swig 3, the tests pass now and we know about at least one person that is using Swig 3 successfully in production. I think that we can move forward with this PR.

@timothy-king
Copy link
Contributor

What is the status of swig 3 on some of the common linux distributions? Debian stable being the likely slow track. (Also mac?) We also might choose to not care about supporting slower moving platforms for being able to compile language bindings at this point. If we need to make that choice, I would rope in @barrettcw .

@mpreiner
Copy link
Member

mpreiner commented Aug 6, 2018

Debian stable has swig3, which was introduced with Debian stretch last year.

@4tXJ7f
Copy link
Member Author

4tXJ7f commented Aug 7, 2018

@barrettcw gave his approval just now. macOS (homebrew) does not even provide Swig 2 anymore, we had to provide our own formula for that, so Swig 3 makes things easier.

@4tXJ7f 4tXJ7f removed the do not merge Do not merge this PR if you are not the author label Aug 7, 2018
@4tXJ7f 4tXJ7f merged commit c831d34 into cvc5:master Aug 8, 2018
@4tXJ7f 4tXJ7f deleted the rm_undefined branch August 8, 2018 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Priority simple Complexity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants