zlib: reduce code duplication

[jhofstee]

The offset in the allocated memory was calculated in alloc and free, this makes it a single constant so it only needs to be defined once.

As requested by @Flarna in #57727

[addaleax]

Isn't this just always alignof(max_align_t) anyway?

[jhofstee]

no, if you don't care about unaligned accesses you could set it to 1. There is no architecture actually doing that though. But this is just addressing a comment made by @Flarna.

[jhofstee]

see #57727 (comment) as well, this just removes a double definition of a constant, there is nothing special about this.

[addaleax]

I'm confused, you're saying "no" here but the comment you linked seems to unambiguously answer my question with "yes"? 🙂

Either way, I don't really care, but I'm not sure that this code is clearer because when reading it it really should raise a question of "why are we defining this constant as the maximum of two values where one is always clearly larger than the other"

[Flarna]

I'm not aware of any platform where alignof(max_align_t) is smaller then sizeof(size_t) as most hardware prefers aligned data.
But to my understanding C++ standard allows alignof(max_align_t) == 1 but the space needed here is sizeof(size_t) so take the max.

[jhofstee]

@addaleax Just make a large struct or class and you can easily check that the alignment of it is less then its size. This code is simply doing the right thing, it will make sure there is enough space and it is still aligned. In theory alignof(max_align_t) can be less than sizeof(size_t). In practice it won't, but the compiler will optimize it out anyway.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.20%. Comparing base (67786c1) to head (1051f9e).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57810      +/-   ##
==========================================
- Coverage   90.21%   90.20%   -0.02%     
==========================================
  Files         630      630              
  Lines      185524   185522       -2     
  Branches    36387    36375      -12     
==========================================
- Hits       167378   167357      -21     
- Misses      11037    11044       +7     
- Partials     7109     7121      +12     

Files with missing lines	Coverage Δ
src/node_zlib.cc	78.37% <100.00%> (-0.05%)	⬇️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Flarna]

Suggested change

	static constexpr size_t reserveSizeAndAlign =
	static constexpr size_t kReserveSizeAndAlign =

Not 100% sure but I think this doesn't fit the code style, it seems inconsistent already in the codebase
Quite some places use a k prefix, others use define style (e.g. RESERVER_SIZE_AND_ALIGN).

[jhofstee]

that sounds like pure evil, are you suggestion to make it a #define in a c++ project?

[Flarna]

Sorry, I didn't mean to convert it to a define, constexpr is perfectly fine and the way to go.

It's just about nitpicking about the name. Unfortunatelly cpp-style-guide doesn't clarify this.

Looking into code I found several places using the k prefix (e.g. here), and some using ALL_UPPER_CASE (e.g. here).

Personally I prefer the proposed k prefix.

[jhofstee]

Understood, I guess I can rename it to reserveSizeAndAlign_, although rather ugly, that seems to comply with the code standard / general implementations. I doubt I have ever seen a k though, unless in kilo. Why would you prefer a k there?

[Flarna]

I don't know where the k prefix comes from but it is used at quite some places in this repo for constants. Therefore I see it as helpful as it jumps into my mind that this is a constant. Would I go for k prefix in my personal repo: no - but thats a different topic.
The casing you use now is used nowhere in this repo for constants.

Aynhow, all I'm asking is to try to follow the existing style8s). I guess kReserveSizeAndAlign, RESERVE_SIZE_AND_ALIGN, reserver_size_and_align (and maybe more) follow exisiting styles.