Skip to content

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.


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?

@@ -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.

@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