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.

Sign up for GitHub

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

Jump to bottom

Switch from ansi-wl-pprint to the prettyprinter package. #178

Merged
merged 3 commits into from
Dec 3, 2020
Merged

Switch from ansi-wl-pprint to the prettyprinter package. #178

merged 3 commits into from
Dec 3, 2020

Conversation

brianhuffman
Copy link
Contributor

This patch updates the following packages:

  • macaw-base
  • macaw-symbolic
  • macaw-x86
  • macaw-x86-symbolic

@brianhuffman
Copy link
Contributor Author

This probably won't work yet because the git submodule references need to be updated for what4 and crucible, and also there are some additional packages that still need to be ported.

Also, this draft PR should not be merged until the corresponding what4 and crucible PRs that it builds on have been merged into their respective master branches.

@brianhuffman
Copy link
Contributor Author

The CI log also has a very fast failure with an error message, "rejecting: prettyprinter-1.7.0 (constraint from project config TODO requires ==1.6.2)". So we'll have to do something about that.

@brianhuffman
Copy link
Contributor Author

I should note that this PR depends on GaloisInc/what4#77 and GaloisInc/crucible#586.

@travitch
Copy link
Contributor

Regarding CI - the Github Actions use freeze files that fix an older version of prettyprinter

brianhuffman pushed a commit to GaloisInc/saw-script that referenced this pull request Nov 25, 2020
Update what4, crucible, and macaw to use prettyprinter package.
c5155ee 
This commit includes the following submodule PRs:
- GaloisInc/what4#77
- GaloisInc/crucible#586
- GaloisInc/macaw#178

WARNING: Do not merge this commit into master until all of the
above PRs are merged into their respective master branches and
the submodule references of this commit can be patched up.
joehendrix
joehendrix reviewed Nov 25, 2020
View reviewed changes
base/src/Data/Macaw/AbsDomain/AbsState.hs Outdated
pretty (CodePointers s b) = text "code" <+> ppSet (s0 ++ sd)
where s0 = if b then [text "0"] else []
pretty (BoolConst b) = viaShow b
pretty (FinSet s) = pretty "finset" <+> ppIntegerSet s
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to just rely on OverloadedStrings and use "finset" since it looks like prettyprinter Doc implements IsString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does implement IsString. My default approach has been to change text "foo" to "foo" if the module already had OverloadedStrings enabled, and to use pretty "foo" otherwise. But I can certainly go back and enable OverloadedStrings in those modules instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Enabling OverloadedStrings would be my preference.

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 have a commit that enables OverloadedStrings on a separate branch (doc-overloaded-strings). I just wanted to get CI to pass before I cherry-pick that commit back onto this PR.

@joehendrix
Copy link
Contributor

Yes, I think just updating the frozen files to the latest versions of packages would be fine. If that ends up being a pain, I'm happy to help out with fixing any errors in a separate PR.

brianhuffman pushed a commit to GaloisInc/saw-script that referenced this pull request Nov 30, 2020
Update what4, crucible, and macaw to use prettyprinter package.
f059f54 
This commit includes the following submodule PRs:
- GaloisInc/what4#77
- GaloisInc/crucible#586
- GaloisInc/macaw#178

WARNING: Do not merge this commit into master until all of the
above PRs are merged into their respective master branches and
the submodule references of this commit can be patched up.
@brianhuffman brianhuffman mentioned this pull request Dec 1, 2020
Merged
brianhuffman pushed a commit to GaloisInc/saw-script that referenced this pull request Dec 1, 2020
Update what4, crucible, and macaw to use prettyprinter package.
c67dd50 
This commit includes the following submodule PRs:
- GaloisInc/what4#77
- GaloisInc/crucible#586
- GaloisInc/macaw#178

WARNING: Do not merge this commit into master until all of the
above PRs are merged into their respective master branches and
the submodule references of this commit can be patched up.
brianhuffman pushed a commit to GaloisInc/saw-script that referenced this pull request Dec 1, 2020
Update what4, crucible, and macaw to use prettyprinter package.
4b56bee 
This commit includes the following submodule PRs:
- GaloisInc/what4#77
- GaloisInc/crucible#586
- GaloisInc/macaw#178

WARNING: Do not merge this commit into master until all of the
above PRs are merged into their respective master branches and
the submodule references of this commit can be patched up.
brianhuffman pushed a commit to GaloisInc/saw-script that referenced this pull request Dec 1, 2020
Update what4, crucible, and macaw to use prettyprinter package.
ad18e0a 
This commit includes the following submodule PRs:
- GaloisInc/what4#77
- GaloisInc/crucible#586
- GaloisInc/macaw#178
- GaloisInc/elf-edit#20

WARNING: Do not merge this commit into master until all of the
above PRs are merged into their respective master branches and
the submodule references of this commit can be patched up.
@brianhuffman brianhuffman mentioned this pull request Dec 2, 2020
Merged
@brianhuffman
Copy link
Contributor Author

I think I'm pretty close to getting CI to pass. It turns out that I needed to patch yet another submodule: GaloisInc/asl-translator#28

@brianhuffman brianhuffman force-pushed the prettyprinter branch 4 times, most recently from b9cf8aa to f73730e Compare December 2, 2020 05:54
@brianhuffman brianhuffman marked this pull request as ready for review December 2, 2020 13:25
@brianhuffman
Copy link
Contributor Author

The tests for 8.6.5 and 8.10.2 are now passing! However the test for ghc 8.8.4 is still failing because it looks like cabal doesn't yet know about lumberjack-0.1.0.3 for some reason.

I'm also waiting to get GaloisInc/asl-translator#28 merged so that I can point this branch at the submodule hash for the merge commit, and then I can apply the OverloadedStrings patch.

@brianhuffman brianhuffman mentioned this pull request Dec 2, 2020
Merged
Switch from ansi-wl-pprint to the prettyprinter package.
2a620d4 
This patch relies on the following submodule updates:
- GaloisInc/what4#77
- GaloisInc/elf-edit#20
- GaloisInc/crucible#586
- GaloisInc/asl-translator#28

This patch updates the following packages:
- macaw-base
- macaw-symbolic
- macaw-x86
- macaw-x86-symbolic
- macaw-aarch32
- macaw-ppc
- macaw-semmc
- macaw-refinement
@joehendrix
Copy link
Contributor

I think if you recreate the cabal ghc-8.8.4.freeze file with an updated cabal, this will generate correctly now. I just tested this on my machine and it seems to work -- I can create a patch if you'd prefer.

@brianhuffman
Copy link
Contributor Author

@joehendrix: The OverloadedStrings patch is in the PR now, and the tests are all passing. Does everything look good to you?

@joehendrix
Copy link
Contributor

Yes. I'm signed off on this. Thanks.

@brianhuffman brianhuffman merged commit 7761a6f into master Dec 3, 2020
brianhuffman pushed a commit to GaloisInc/saw-script that referenced this pull request Dec 3, 2020
Update saw-script and submodules to use prettyprinter package.
f1ae175 
This commit includes the following submodule PRs:
- GaloisInc/what4#77
- GaloisInc/crucible#586
- GaloisInc/macaw#178
- GaloisInc/elf-edit#20
@Ptival Ptival deleted the prettyprinter branch August 14, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joehendrix joehendrix joehendrix left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brianhuffman @travitch @joehendrix