New issue
Advanced search Search tips

Issue 796470 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 772870



Sign in to add a comment

Clean zlib inflate chunk copy code of compiler warnings.

Project Member Reported by noel@chromium.org, Dec 20 2017

Issue description

Changing the zlib BUILD.gn rule for the inflate chunk copy code to use the much less permissive chromium compiler:chromium_code config (diff follows)

 source_set("zlib_inflate_chunk_simd") {
   ...
-  configs -= [ "//build/config/compiler:chromium_code" ]
-  configs += [ "//build/config/compiler:no_chromium_code" ]
-
   public_configs = [ ":zlib_inflate_chunk_simd_config" ]
 }

produces warnings:

../../third_party/zlib/contrib/optimizations/chunkcopy.h:114:19: error: comparison of integers of different signs: 'long' and 'unsigned long' [-Werror,-Wsign-compare]
  if (limit - out < CHUNKCOPY_CHUNK_SIZE) {
      ~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~

../../third_party/zlib/contrib/optimizations/chunkcopy.h:399:19: error: comparison of integers of different signs: 'long' and 'unsigned long' [-Werror,-Wsign-compare]
  if (limit - out < CHUNKCOPY_CHUNK_SIZE * 3) {

Since zlib is compiled in C, correct, these are errors.  

The left side of the expression is ptrdiff_t type, and the right side of the expression should be ptrdiff_t also.  But it is size_t.

We should fix the comparison, and use compiler:chromium_code more to prevent similar errors slipping into the code in future.
 

Comment 1 by noel@chromium.org, Dec 20 2017

Owner: noel@chromium.org
Status: Started (was: Untriaged)

Comment 2 by noel@chromium.org, Dec 20 2017

Comparison using ptrdiff_t removes the warning, but could not use the less lenient compiler:chromium_code rule due to "too many old style declaration warnings" from the MSVC release bot when I tried that.  So just do the ptrdiff_t part.

Comment 3 by noel@chromium.org, Dec 20 2017

Note the warnings are from the other code added to the inflate chunk copy source set in BUILD.gn, namely inflate.c, inffast.*, yada yada.

Comment 4 by noel@chromium.org, Dec 20 2017

Blocking: 772870
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 20 2017

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

commit f15491d8649d1fc27ae2d0d832c9bb6813b04ea0
Author: Noel Gordon <noel@chromium.org>
Date: Wed Dec 20 12:22:09 2017

zlib inflate chunk copy code: fix compiler warnings

Comparison of a pointer difference to a size_t cause the compiler
to whine about the different signs: 'long' and 'unsigned long' in
this case. Fix that by using ptrdff_t for these comparisons.

No change in behavior, no new tests.

Tbr: mtklein@chromium.org
Bug:  796470 
Change-Id: Ie585ba45f1e09f31f7d4cf48c41f2378a1b0808b
Reviewed-on: https://chromium-review.googlesource.com/836291
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525312}
[modify] https://crrev.com/f15491d8649d1fc27ae2d0d832c9bb6813b04ea0/third_party/zlib/contrib/optimizations/chunkcopy.h

Comment 6 by noel@chromium.org, Dec 20 2017

Status: Fixed (was: Started)

Sign in to add a comment