Partially fix #611 with Cow #743
Conversation
The Text variant of Document now contains a Cow<str> instead of a String. This small modification triggered a lot of lifetime and type errors accross the codebase. This commits fixes these errors, in the most conservative way possible. Tests have been fixed too, so that they still compile (and pass). Next commits will focus on completely removing the usage of String in Document.
This commit removes every usage of String in Document, replacing it with the Cow<str> type. The work is not finished yet! A lot of clone still have to be removed. This will be the goal of the next few commits. Tests have been updated, and still pass.
The call to clone was removed in the implementation of Documentable for &str. Warning: the code currently does not compile. The compilation errors are due to pattern-matching. These errors *should* be fixed with the `butcher` crate, which *should* significantly reduce the amount of duplication generated by pattern-matching on enums wrapped in Cow.
Butcher will make Cow handling a bit less painfull, and will remove a lot of code duplication.
|
I'm a bit worried by the fact that CI fails for MacOS, but not for other targets. Also the provided error seems to be What am I missing? |
This has happened to me before too on the MacOS CI. It was fixed when Louis reran the actions. Looks like an open issue on GitHub's end :) |
This conflict comes from commit d4e11ea.
Thanks for your comment @thehabbos007! Fixing merge conflicts triggered CI, which successfully pass :) |
There was a problem hiding this comment.
Thanks for taking this on.
From reading the butcher crate I don't understand what exactly it does, and I'm reluctant to introduce a library that has such a wide impact on the codebase. I don't think that a performance optimisation in the pretty printers should require changes throughout the compiler which result in all contributors learning a new library + set of conventions.
Is it possible to use the library in a fashion that only impacts the algebra? At the moment this seems like a larger change than having Document::String and Document::Str variants, while also being more conceptually complex
|
Hi, To be honest, I share your opinion in the fact that this PR has way to much impact in the codebase. This is not directly caused by the Anyway, I do think that other developers are not wiling to learn the butcher crate works. After some reflection, I am pretty sure I misunderstood what should be done to address such issue, and that I went way too deep in the codebase to fix it (hence the huge amount of changes). As such, I'll come back in a few days in the issue thread to ask more questions about what should be done. This issue was opened a long time ago, and there I still have not properly fixed it. I do thank you for your infinite patience here. |
Please do not feel bad in any way! I don't expect any particular contribution to Gleam from anyone and I do not think I am entitled to anyone's time. I am thankful for your contributions (even if we haven't figured this specific problem out yet) and I have learnt quite a few things from reading your Rust code :) Work on whatever is fun, that is the most important thing for open source work in my opinion :) |
This PR attempts to eliminate some useless copies. It introduced a lot of lifetime constraints which I tried to resolve in the most conservative way. In a lot of situation, this resulted in heavy use of
Cow, which introduced type errors. That's why this pull request is very big. I was unfortunately unable to split it into smaller PR.Feel free to post a comment if you see a situation in which a modification was not necessary.
The
butchercrate has been used to remove most of the duplicate introduced in a previous attempt to fix this issueThis partially fix #611. There is still some work to do, such as removing as much calls to
unbutcheras possible.