zlib: Use defines for inffast |
|||||
Issue descriptionZlib'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.
,
Sep 12 2017
,
Sep 12 2017
,
Sep 14 2017
Patch is at: https://chromium-review.googlesource.com/c/chromium/src/+/663424
,
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
,
Sep 21 2017
,
Feb 10 2018
Re-opening to first land a change to remove the .patch file.
,
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
,
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)
,
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
,
Feb 12 2018
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
,
Feb 12 2018
> "I guess we could do an #undef" Works for me.
,
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
,
Feb 13 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by cavalcantii@chromium.org
, Sep 12 2017