Wrap the Shake functions with newtypes #1753
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pepeiborra
reviewed
Apr 18, 2021
pepeiborra
approved these changes
Apr 18, 2021
pepeiborra
pushed a commit
to pepeiborra/ide
that referenced
this pull request
Apr 23, 2021
* Add a function addRule, which abstracts over addBuiltinRule * Newtype wrap the Action, Rules and ShakeOption types * Replace addShakeExtra with newShakeExtra * Move the internals of hls-graph around a bit * Expose shakeTimings and shakeAllowRedefineRules through ShakeOptions Co-authored-by: Potato Hatsue <1793913507@qq.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To ensure that we really are using our own types, and our own library, it's useful to newtype wrap the Action, ShakeOptions and Rules types. That means that if we accidentally use Shake, it's a type error. It's also step 1 towards replacing Shake, but even if we don't, it makes the abstraction more robust.
In order to further enhance the abstraction, I've changed
addBuiltinRuleforaddRulewhich doesn't do the linting and tracking stuff, and simplified the "extra" feature in Shake to only allow a single value, as that's all we need. Fairly minor changes, but cut down on the complexity of the interface to Shake, where we're only using a subset.There's a potential performance cost to newtype wrapping when it's a list, but if we had to we could switch to the newtype coercion stuff, and these aren't performance sensitive bits, so it's will be negligible. (I ran a benchmark, and saw no change beyond random variation.)
I haven't tried to wrap all Shake types, as that doesn't really give any benefits, but that might be a reasonable end goal in the future.