RazrFalcon / rustybuzz

A complete harfbuzz's shaping algorithm port to Rust
MIT License
498 stars 34 forks source link

Uglify. #99

Closed RazrFalcon closed 4 months ago

RazrFalcon commented 4 months ago

Make codebase look as close to harfbuzz as possible.

RazrFalcon commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Then it's fine for me!

bluebear94 commented 4 months ago

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 commented 4 months ago

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

DemiMarie commented 4 months ago

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

RazrFalcon commented 4 months ago

Yeah, I wish...

DemiMarie commented 4 months ago

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

RazrFalcon commented 4 months ago

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

LaurenzV commented 4 months ago

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