aisamanra / s-cargot

Elaborate and expressive S-Expression library for Haskell
Other
61 stars 6 forks source link

New faster pretty-printer #9

Closed kquick closed 6 years ago

kquick commented 6 years ago

This is an alternate approach to the pretty printer. The primary focus is on speed, so it pre-processes the SExpr tree to produce an alternate form with atoms converted to their text format and the length determined; the main pass can then use these unconstrained lengths as initial indications of whether wrapping is needed and then perform the wrapping using a cursor that maintains the current indentation, remaining width, and current indentation level.

This code is even more deserving of the "yuck" comment, but it reduces the time for the test sample from 30+ minutes of 100% CPU time (estimated... I wasn't that cruel to my system) to about 0.193s.

Also, the indentation determination has lost the original SExpr data by the time it is called, so it just passes an arbitrary simple SExpr to get back the indentation type; this is not ideal, but should handle most of the cases.

kquick commented 6 years ago

The latest work here adds a LetBind helper module which will allow the introduction of let-bind variable substitution and the corresponding expansions. This is not strictly tied to the pretty-printing work, but the original pretty-printer is pretty much unusable for non-trivial S-expressions, so I'm submitting this as part of this pull request; let me know if you need this submitted separately.

The default weighting to identify most-probable candidate phrases for let binding is not necessarily optimal; the intention is that the caller can supply a more knowledgeable version to achieve better bindings based on the associated language.

aisamanra commented 6 years ago

I was slow to look these over because I wanted to read and understand the pretty-printing code before doing anything, so I'm going to finish the merge this afternoon.

To be honest, I'm not sure about merging the LetBind module at all into s-cargot proper. Not because it's not useful—it definitely is!—but because it feels comparatively specific to a relatively narrow use-case that doesn't entirely overlap with the stated use-case of s-cargot—for example, there's no real guarantee that it'll make sense to use on programming language sources or configuration formats, even if those formats have a let construct—and it's also defined entirely in terms of exported externals and thus could easily exist external to s-cargot itself. If it's okay with you, I think it might be better to split that module into a separate s-cargot-letbind package, which I'd be happy to do if you don't feel like doing extra Cabal legwork!

kquick commented 6 years ago

No problem, this was getting messy with entanglement anyhow. I'll generate a new pull request with just the pretty printing updates.