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

add Data.Align.Text #1

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

add Data.Align.Text #1

wants to merge 2 commits into from

Conversation

ocramz
Copy link

@ocramz ocramz commented Apr 9, 2020

  • Add 'text' dependency
  • Add 'CHANGELOG.md'
  • Moved common types to Data.Align.Types
  • Add type signatures in Data.Align.Demo
  • Small cosmetic refactorings in Data.Align
  • bump package version to 0.2

Copy link
Owner

@robinp robinp left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Minor comments, looks good otherwise.

Optional for bonus points (in separate PR): think up & add some property tests with hedgehog.

stack.yaml Outdated Show resolved Hide resolved

tappend :: (Functor f, Num s) =>
f (Trace a s) -> Trace a s -> f (Trace a s)
mt `tappend` (Trace z (t:_)) =
Copy link
Owner

Choose a reason for hiding this comment

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

Please move to be a local helper at callsite, as mentioned in #2 .

Copy link
Author

Choose a reason for hiding this comment

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

Good point, but we're now using this in Data.Align.Text as well. Perhaps an INLINE pragma would work instead?

src/Data/Align/Types.hs Outdated Show resolved Hide resolved
@@ -0,0 +1,60 @@
{-# language RecordWildCards #-}
Copy link
Owner

Choose a reason for hiding this comment

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

If you have even anecdotal knowledge, please add a few lines of comment about expectable gains vs using stringy version.

Copy link
Author

Choose a reason for hiding this comment

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

I think better still would be to set up a benchmark suite.

src/Data/Align/Text.hs Outdated Show resolved Hide resolved
@ocramz
Copy link
Author

ocramz commented Apr 11, 2020

Now I'm not even too sure about duplicating align, since the only differences in the implementations are the implementations of index and length. Perhaps using a typeclass with instances for Vector, String and Text would be better?

@ocramz ocramz marked this pull request as draft April 11, 2020 07:08
@robinp
Copy link
Owner

robinp commented Apr 12, 2020

Is the duplication driven by an actual performance need? If not, can as well keep the listy module only, and convert at callsite.

Otherwise, doing benchmarks on the real problem (against the simply-wrapping) with criterion indeed sounds good.

Not sure about the typeclass. A bit hackier, but an often employed trick (see containers IIRC) is to have a header file contain the code (with some macro placeholders), and include it with CPP from the various implementations.

Personally I never liked the trick, but the typeclass move is something to be made only if benchmark data verifies its positive effect (passing around dictionaries and whatnot).

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.

2 participants