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

Modify the SMT-LIB printer to declare arrays in a deterministic (alphabetical) order. #106

Merged
merged 2 commits into from
Apr 3, 2014

Conversation

pcc
Copy link
Contributor

@pcc pcc commented Mar 18, 2014

I also reformatted the SMT-LIB printer with clang-format for consistency with the rest of KLEE.

@delcypher
Copy link
Contributor

Thanks peter. I had been meaning to reformat that code for a while.

The code looks fine at a glance. I'm curious though, what is the motivation for wanting arrays declared in a deterministic order?

@MartinNowack
Copy link
Contributor

Hi @ppc, @delcypher,

I'm not a big fan of re-format-only patches (beside the header is now broken, have a look at http://llvm.org/docs/CodingStandards.html#commenting; comments are broken; ...).
Let's change format as we go.

But thanks for the other patch - looks good to me as well.

@pcc
Copy link
Contributor Author

pcc commented Mar 19, 2014

@delcypher At least two that I can think of: one is that it makes testing the SMT-LIB printer easier (as you can rely on the arrays being declared in a consistent order), the other is that it allows for (potentially expensive) queries to be automatically deduplicated based on the text of the query.

@MartinNowack Sorry, but I really don't like the existing formatting. I have more changes to the SMT-LIB printer in the pipeline that will probably end up moving code around and I'd rather take care of all the reformatting in one go at the start. I can probably go back and clean up some of the comments that clang-format misformatted.

@ccadar
Copy link
Contributor

ccadar commented Mar 19, 2014

Thanks for the patch, @pcc, I agree it's a useful change. Regarding the formatting, as you know, the basic rule is, citing LLVM standards, that "If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow." Otherwise, if everyone reformatted each file in which they send a patch, things would become quite messy. This being said, if the original author of that file, @delcypher in this case, agrees with the reformatting, I am happy to change it too (and in general, I think we all agree that we should start formatting new code using the LLVM style, and clang-format looks like a useful tool).

@delcypher
Copy link
Contributor

@pcc Sorry for taking a while at looking at this. I was going to merge but I noticed this has broken the Expr/print-smt.pc test. Can you please fix the test? We've switched to using lit now so hopefully you'll find running tests in KLEE much easier than it used to be :)

Once you've done this I'll merge.

@pcc
Copy link
Contributor Author

pcc commented Apr 3, 2014

Done.

@delcypher
Copy link
Contributor

Thanks.

delcypher added a commit that referenced this pull request Apr 3, 2014
Modify the SMT-LIB printer to declare arrays in a deterministic (alphabetical) order.
@delcypher delcypher merged commit b3d9f14 into klee:master Apr 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants