New issue
Advanced search Search tips

Issue 764431 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 764085



Sign in to add a comment

zlib: Use defines for inffast

Project Member Reported by cblume@chromium.org, Sep 12 2017

Issue description

Zlib's inffast uses a handful of magic numbers. It would be easier to read and maintain if these magic numbers were instead #defines.

NigelTao@ wrote this code to clean up the magic numbers in https://chromium-review.googlesource.com/c/chromium/src/+/601694

Nigel gave me permission to make a separate pull request to separate out just the magic number cleaning.
 
Indeed, it was a nice change introduced by Nigel and I agree it makes sense to introduce it separately.

Blocking: 764085
Linking it to the master zlib bug.
Status: Started (was: Assigned)
Patch is at:
https://chromium-review.googlesource.com/c/chromium/src/+/663424
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5

commit 5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5
Author: Chris Blume <cblume@google.com>
Date: Tue Sep 19 23:07:57 2017

Zlib: Use defines for inffast

Zlib's inffast uses a handful of magic numbers. It would be easier to
read and maintain if these magic numbers were instead #defines.

NigelTao@ wrote this code to clean up the magic numbers in
https://chromium-review.googlesource.com/c/chromium/src/+/601694

Nigel gave me permission to make a separate pull request to separate out
just the magic number cleaning.

BUG= 764431 

Change-Id: I0a62e31e98d4f3afcc64bd096e62a4b4b175644b
Reviewed-on: https://chromium-review.googlesource.com/663424
Commit-Queue: Chris Blume <cblume@google.com>
Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502982}
[add] https://crrev.com/5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5/third_party/zlib/0003-use-defines-for-inffast.patch
[modify] https://crrev.com/5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5/third_party/zlib/README.chromium
[modify] https://crrev.com/5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5/third_party/zlib/infback.c
[modify] https://crrev.com/5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5/third_party/zlib/inffast.c
[modify] https://crrev.com/5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5/third_party/zlib/inffast.h
[modify] https://crrev.com/5d7c2b78db0e96c769dee8d4ed3a9aeac8ef21d5/third_party/zlib/inflate.c

Status: Verified (was: Started)

Comment 7 by noel@chromium.org, Feb 10 2018

Cc: scroggo@chromium.org mtklein@chromium.org
Status: Started (was: Verified)
Re-opening to first land a change to remove the .patch file.
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f53ef9e9a51e6b4d16e50ff6533d3761cacb868e

commit f53ef9e9a51e6b4d16e50ff6533d3761cacb868e
Author: Noel Gordon <noel@chromium.org>
Date: Sat Feb 10 01:41:35 2018

Remove zlib/patches/0003-use-defines-for-inffast.patch

Patch files are no longer needed per the README.chromium guidelines.

Bug:  764431 
Change-Id: I7cbfb9e51f74a7499697e6ff541bb02e30572c5b
Reviewed-on: https://chromium-review.googlesource.com/910870
Reviewed-by: Chris Blume <cblume@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535919}
[delete] https://crrev.com/19db638f87e6f7805b24e996aa01f340ac8ecc4f/third_party/zlib/patches/0003-use-defines-for-inffast.patch

Comment 9 by noel@chromium.org, Feb 10 2018

Planning discussions in code review are probably not the best forum + the current bug was marked verified fixed, and so no next steps indicated.

If you have plans, my dear chromium developers, please begin by at least writing chromium bug since I cannot read your minds, but I can reply to bugs.  From the code review of #8

cblume@
> Sorry, my message was incomplete there.
> I disagree with the HAVE/LEFT define being moved out and now obsolete.
> If there are better names that is cool. But the intent was to get rid of the magic numbers. I don't see the benefit of bringing magic numbers back.

noel@
> I dunno Chris, this brings root back to pristine state which would simplify a merge upstream.  The magic numbers are a don't care to me since we don't any of these files in chrome no more, but you might like to maintain them?
> Do we need to keep the .patch file?

cblume@
> Of all the zlib work we've done, this is probably the most likely to be accepted upstream. It provides value to upstream and is very simple. Plus, it shows we care about zlib as a whole and not just the parts that benefit us. This is important if we want to convince Mark to consider accepting additional help for accepting upstream patches.

noel@
> Ok, I'm gonna assume the answer to "but you might like to maintain them?" is yes.  Perhaps one change to avoid confusion, would be cargo cult the comments and #define names we use in the contrib versions of these files back into the root?

cblume@
> I'm okay getting rid of the .patch though.

noel@
> Cool, let me do just that (see #8 submitted)

Comment 10 by noel@chromium.org, Feb 12 2018

If this work is to become a litmus test for upstreaming to canonical zlib, I wasn't aware of of it.  What I was simply noting in code review is not that we don't care, but that we're not using this patch chrome zlib as of today.  If we want to take it upstream, ok to carry this patch in chrome I suppose, but I'd love to see it happen (acceptance upstream) to see that that idea is gonna work fo realz, AI #1.

Perhaps before we do that, we might want to consider backporting the new names and their commentary from the contrib/ version of this patch [1], back into the zlib root directory, AI #2.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/900885
Yeah, I think it makes sense to use the names from root in contrib, since they are supposed to be the same thing. Over in this patch [1] the value changes for contrib, though. I guess we could do an #undef

[1] https://chromium-review.googlesource.com/c/chromium/src/+/900982/8/third_party/zlib/contrib/optimizations/inffast_chunk.h

Comment 12 by noel@chromium.org, Feb 12 2018

> "I guess we could do an #undef"  Works for me.

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46f115f22260093ee7bc2a8764adf64d9c48a612

commit 46f115f22260093ee7bc2a8764adf64d9c48a612
Author: Chris Blume <cblume@chromium.org>
Date: Tue Feb 13 19:57:43 2018

Share inffast names in zlib

We use a different set of names in root and contrib directories for the
same purpose.

This patch consolidates the names.

BUG= 764431 

Change-Id: I346a6f9ab67423b97843346bab6b0c58f516ed1d
Reviewed-on: https://chromium-review.googlesource.com/914469
Commit-Queue: Chris Blume <cblume@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536429}
[modify] https://crrev.com/46f115f22260093ee7bc2a8764adf64d9c48a612/third_party/zlib/contrib/optimizations/inffast_chunk.h
[modify] https://crrev.com/46f115f22260093ee7bc2a8764adf64d9c48a612/third_party/zlib/infback.c
[modify] https://crrev.com/46f115f22260093ee7bc2a8764adf64d9c48a612/third_party/zlib/inffast.c
[modify] https://crrev.com/46f115f22260093ee7bc2a8764adf64d9c48a612/third_party/zlib/inffast.h
[modify] https://crrev.com/46f115f22260093ee7bc2a8764adf64d9c48a612/third_party/zlib/inflate.c

Status: Fixed (was: Started)

Sign in to add a comment