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

Implement display_list #738

Merged
merged 8 commits into from
Aug 23, 2020
Merged

Implement display_list #738

merged 8 commits into from
Aug 23, 2020

Conversation

thomastanck
Copy link
Member

@thomastanck thomastanck commented Aug 22, 2020

Resolves #379, and also changes the standard stringify of lists slightly, see eac6949.

The implementation does two passes over the list structure. It first performs a deep copy of the entire list structure, and at the same time identifies nodes which are list-like by constructing a ListObject for it. On the first pass, we keep all references as references to the original list structure for simplicity. In a second pass, it then replaces references to the old list structure, using set_head/tail to point these to the new list structure. We don't process the same pair more than once, so runtime complexity is linear in the number of pairs in the list structure.

The implementation assumes that the whole thing is a pure list structure, and there are no objects. To fix this for objects is not too tricky: we just have to add more special cases to the top of the visit function, to have the visit function recursively call object members, and also recreate objects during the second pass.

This PR also exposes a new interface similar to toReplString, that we use to implement the pretty printing. Now, if an object contains the properties replPrefix, replSuffix, and replArrayContents, it will assume the object is "array-like", and will stringify the contents and join them very similarly to how it handles pairs/arrays. This means formatting logic (e.g. multiline handling) is kept within stringify, and can be changed more easily if we need to.

This raises the possibility of perhaps pretty printing runes created by beside, etc. as those are also "array-like".

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan
Used to have lots of annoying spaces at the end of a multiline list.
I've gotten rid of those spaces so the end of lists are simply suffixed
with ']'.
@coveralls
Copy link

coveralls commented Aug 22, 2020

Pull Request Test Coverage Report for Build 3805

  • 69 of 69 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 83.364%

Totals Coverage Status
Change from base Build 3784: 0.1%
Covered Lines: 6829
Relevant Lines: 7900

💛 - Coveralls

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan
@martin-henz
Copy link
Member

Played a bit. Looks really solid. Good job @thomastanck ! Will try to find a reviewer.

Copy link
Contributor

@openorclose openorclose left a comment

Choose a reason for hiding this comment

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

other than the infinite lists, let's have some tests of list of pairs, to make sure they display properly like

list(pair(1, 2), pair(3, 4));

return (
typeof v.replPrefix !== 'undefined' &&
typeof v.replSuffix !== 'undefined' &&
typeof v.replArrayContents !== 'undefined'
Copy link
Contributor

Choose a reason for hiding this comment

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

since it needs to be a function

Suggested change
typeof v.replArrayContents !== 'undefined'
typeof v.replArrayContents === 'function'

maybe can check that the prefix and suffix are strings instead too

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan
Works by measuring a bunch of size vs runtime points, and doing linear
regression on a log-log plot, using the resulting gradient as an
estimate for the power of the runtime complexity.

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan
replPrefix and replSuffix should be strings,
replArrayContents should be a function.

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan

Verified

This commit was signed with the committer’s verified signature.
thomastanck Thomas Tan
@openorclose openorclose merged commit d6c141f into master Aug 23, 2020
@thomastanck thomastanck deleted the tt/display_list branch August 24, 2020 23:36
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.

LISTS: add display_list to improve the display of lists in the REPL
4 participants