Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(207)

Issue 1605153004: unittests: fix -Wnarrowing build errors (Closed)

Created:
4 years, 11 months ago by vapier
Modified:
4 years, 11 months ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

convert to uint8_t* for binary data to fix -Wnarrowing build errors Newer gcc versions default to -Werror=narrowing when using newer C++ standards (which we do). This causes issues when we try to stuff a value like 0xea into a char -- the value is out of range for signed char bytes. That's when gcc throws an error: .../bytereader_unittest.cc: In member function 'virtual void Reader_DW_EH_PE_absptr4_Test::TestBody()': .../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from 'int' to 'char' inside { } [-Wnarrowing] BUG=chromium:579384 TEST=`make check` passes R=mark@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/258591ed4cb502925f1ffda8c8af57559c0c1c26

Patch Set 1 #

Patch Set 2 : convert to uint8_t #

Total comments: 4

Patch Set 3 : address some feedback #

Patch Set 4 : fix one more long line #

Total comments: 1

Patch Set 5 : switch to cstdint #

Patch Set 6 : back to stdint.h #

Unified diffs Side-by-side diffs Delta from patch set Stats (+219 lines, -173 lines) Patch
M src/common/dwarf/bytereader.h View 1 2 3 4 5 9 chunks +17 lines, -13 lines 0 comments Download
M src/common/dwarf/bytereader.cc View 1 2 3 4 5 4 chunks +4 lines, -3 lines 0 comments Download
M src/common/dwarf/bytereader-inl.h View 1 2 3 4 5 6 chunks +12 lines, -17 lines 0 comments Download
M src/common/dwarf/bytereader_unittest.cc View 1 2 3 4 5 20 chunks +30 lines, -20 lines 0 comments Download
M src/common/dwarf/dwarf2diehandler.h View 1 2 3 4 5 3 chunks +4 lines, -2 lines 0 comments Download
M src/common/dwarf/dwarf2diehandler.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M src/common/dwarf/dwarf2diehandler_unittest.cc View 1 2 3 4 5 4 chunks +7 lines, -4 lines 0 comments Download
M src/common/dwarf/dwarf2reader.h View 1 2 3 4 5 14 chunks +27 lines, -27 lines 0 comments Download
M src/common/dwarf/dwarf2reader.cc View 1 5 26 chunks +44 lines, -42 lines 0 comments Download
M src/common/dwarf/dwarf2reader_cfi_unittest.cc View 1 2 3 4 5 16 chunks +32 lines, -16 lines 0 comments Download
M src/common/dwarf/dwarf2reader_die_unittest.cc View 1 2 3 4 5 3 chunks +6 lines, -3 lines 0 comments Download
M src/common/dwarf_cu_to_module.h View 1 2 3 4 5 3 chunks +4 lines, -2 lines 0 comments Download
M src/common/dwarf_cu_to_module.cc View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments Download
M src/common/dwarf_cu_to_module_unittest.cc View 1 2 3 4 5 6 chunks +7 lines, -5 lines 0 comments Download
M src/common/linux/dump_symbols.cc View 1 2 3 4 5 8 chunks +15 lines, -12 lines 0 comments Download
M src/common/mac/dump_syms.cc View 1 2 3 4 5 5 chunks +5 lines, -4 lines 0 comments Download

Messages

Total messages: 21 (4 generated)
vapier
i will be the first to admit i'm not terribly happy with this internal macro ...
4 years, 11 months ago (2016-01-20 23:53:44 UTC) #2
Mark Mentovai
I hate all of the options too. Is this stuff ever even conceptually char data? ...
4 years, 11 months ago (2016-01-21 21:21:03 UTC) #3
vapier
i can take a look at just how painful it would be to convert everything ...
4 years, 11 months ago (2016-01-21 21:24:00 UTC) #4
brucedawson
> the value is implicitly sign-extended > to an int (0xffffffff) before being truncated back ...
4 years, 11 months ago (2016-01-21 21:38:36 UTC) #5
Mark Mentovai
int does enter into it, because 0xea is a (signed) int. There’s no sign-extension happening, ...
4 years, 11 months ago (2016-01-21 21:49:51 UTC) #6
vapier
i rewrote the dwarf bytereader modules to use uint8_t. was much larger, but looks straight ...
4 years, 11 months ago (2016-01-23 18:42:41 UTC) #8
Mark Mentovai
LGTM Although it touches more lines, this is fairly clean. I’m not too concerned with ...
4 years, 11 months ago (2016-01-25 15:08:43 UTC) #9
vapier
should have all the feedback addressed here. i'll land later today/tomorrow if there's no other ...
4 years, 11 months ago (2016-01-25 21:38:41 UTC) #11
Mark Mentovai
You should probably get <stdint.h> into the other headers here too, but LGTM otherwise.
4 years, 11 months ago (2016-01-25 21:50:04 UTC) #12
Mark Mentovai
https://codereview.chromium.org/1605153004/diff/60001/src/common/linux/dump_symbols.cc File src/common/linux/dump_symbols.cc (right): https://codereview.chromium.org/1605153004/diff/60001/src/common/linux/dump_symbols.cc#newcode1 src/common/linux/dump_symbols.cc:1: // Copyright (c) 2011 Google Inc. And I guess ...
4 years, 11 months ago (2016-01-25 21:50:39 UTC) #13
vapier
i've switched to <cstdint> rather than <stdint.h>, and added it to all these files. i ...
4 years, 11 months ago (2016-01-25 22:14:18 UTC) #14
Lei Zhang
On 2016/01/25 22:14:18, vapier wrote: > i've switched to <cstdint> rather than <stdint.h>, and added ...
4 years, 11 months ago (2016-01-25 22:24:55 UTC) #15
vapier
<cstdint> feels more natural to me in C++ code, but i don't really have a ...
4 years, 11 months ago (2016-01-25 22:35:36 UTC) #16
Lei Zhang
On 2016/01/25 22:35:36, vapier wrote: > <cstdint> feels more natural to me in C++ code, ...
4 years, 11 months ago (2016-01-25 23:05:38 UTC) #17
Mark Mentovai
Please don’t use <cstdint> unless you want to call the type std::uint8_t everywhere.
4 years, 11 months ago (2016-01-25 23:34:48 UTC) #18
vapier
okie dokie, back to stdint.h
4 years, 11 months ago (2016-01-25 23:45:23 UTC) #19
vapier
4 years, 11 months ago (2016-01-26 20:38:25 UTC) #21
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as
258591ed4cb502925f1ffda8c8af57559c0c1c26 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698