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

Uglify. #99

Merged
merged 23 commits into from
Feb 24, 2024
Merged

Uglify. #99

merged 23 commits into from
Feb 24, 2024

Conversation

RazrFalcon
Copy link
Collaborator

@RazrFalcon RazrFalcon commented Feb 17, 2024

Make codebase look as close to harfbuzz as possible.

@RazrFalcon
Copy link
Collaborator Author

RazrFalcon commented Feb 17, 2024

@LaurenzV how about this? I've started with ot_shape_normalize.rs. Function names, functions order are all the same.
It does look ugly as hell, but if you compare harfbuzz and rustybuzz side by side - they look almost identical now. Which is exactly what we want.

@LaurenzV
Copy link
Collaborator

LaurenzV commented Feb 17, 2024

Hmm, yeah that's a very unusual sight for Rust code. xD Not sure whether we really need to change something like "Buffer" as well... But I guess it's better to really go 100% all in into making it exactly like harfbuzz instead of once again just going for a partial refactoring.

Maybe we should use the prefix "rb" (for rustybuzz) instead of "hb"? But it doesn't really matter. Other than that I think I would be okay with this change. It's always possible to later on build another crate that wraps the core of rustybuzz in a more Rust-native way, whilw going the other way around is more difficult.

@RazrFalcon
Copy link
Collaborator Author

Yes, it's very ugly. But that's the problem. I was prioritizing idiomatic code to ease of porting.
Actually, this is exactly how the code looked initially. I did refactoring after I finished porting. That was a wrong move.

As for the rb prefix, it would harm discoverability/search. And all of those ugly naming are fully internal. The public API would not change a bit.
And we do not export any of the internals, so name conflicts are not possible either. In case someone would try to link the original harfbuzz to the same binary.

The goal is to make code look like it was machine generated. For better or worse.

@LaurenzV
Copy link
Collaborator

Then it's fine for me!

@bluebear94
Copy link
Collaborator

bluebear94 commented Feb 17, 2024

Thanks @LaurenzV for the work on porting! I’ve been wanting to do it for a while but couldn’t find the time to do so.

Instead of renaming the items to match HarfBuzz, we could add comments (or the #[doc(alias)] attribute) naming the HarfBuzz equivalent to each item, though this takes a bit more effort to match them up while porting changes.

@RazrFalcon
Copy link
Collaborator Author

@bluebear94 this would solve the search issue, but the function code itself would look drastically different to harfbuzz, which would still be confusing.

@DemiMarie
Copy link

@RazrFalcon I hope that eventually, rustybuzz will become harfbuzz and the original C++ code will be ripped out.

@RazrFalcon
Copy link
Collaborator Author

Yeah, I wish...

@DemiMarie
Copy link

IIRC there is actually interest from Google in this, mostly because of the endless stream of bugs found by fuzzers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure that mentions of Buffer and other types are renamed in .rl files as well!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I will do it later.

@RazrFalcon RazrFalcon merged commit 332d5d4 into master Feb 24, 2024
2 checks passed
@RazrFalcon
Copy link
Collaborator Author

RazrFalcon commented Feb 24, 2024

@LaurenzV @bluebear94 Done. It's far from perfect and there is still huge amount of work left, but it's already looks way closer to harfbuzz than it was before.

The final goal is to have the same file structure, the same functions order (right now some functions are in wrong files) and the same naming.
The code should look like it was auto-translated from C++.

Functions content is way harder, since it's Rust and not C++. For example, we use iterators whenever possible to avoid bounds checking, while harfbuzz relies on unchecked indexing. No reason in replicating it. We only loose performance.

In general, if you see that a function has a different name or in a wrong file or at a wrong position in a file - feel free to "update" it, but preferably as a separate commit.

Any suggestions about how we can simplify porting further are welcome.

@RazrFalcon RazrFalcon deleted the uglify branch February 24, 2024 18:53
@LaurenzV
Copy link
Collaborator

Nice, thanks! I'll have a look within the next few days

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.

4 participants