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

Issue 2124083002: Fix compilation with strict format checking (Closed)

Created:
4 years, 5 months ago by Adam Goode
Modified:
4 years, 5 months ago
Reviewers:
Lei Zhang
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

Fix compilation with strict format checking abs() is bit tricky in C++ and this use of abs is returning double. FXSYS_snprintf is strictly checking this on Fedora 24 and results in: ../../third_party/pdfium/fpdfsdk/fsdk_baseannot.cpp:309:18: error: format specifies type 'int' but the argument has type 'typename __gnu_cxx::__enable_if<__is_integer<signed char>::__value, double>::__type' (aka 'double') [-Werror,-Wformat] Committed: https://pdfium.googlesource.com/pdfium/+/c9a0ec50d813d13498830c05d60d01360729a156

Patch Set 1 #

Total comments: 1

Patch Set 2 : Move static_cast to inside the call to abs #

Patch Set 3 : Use std::abs and static_cast in both tzHour prints #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -3 lines) Patch
M fpdfsdk/fsdk_baseannot.cpp View 1 2 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 28 (8 generated)
Adam Goode
I don't seem to be able to trigger any trybots as I would with chromium. ...
4 years, 5 months ago (2016-07-06 19:59:29 UTC) #2
dsinclair
On 2016/07/06 19:59:29, Adam Goode wrote: > I don't seem to be able to trigger ...
4 years, 5 months ago (2016-07-06 20:06:02 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2124083002/1
4 years, 5 months ago (2016-07-06 20:17:05 UTC) #5
commit-bot: I haz the power
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started ...
4 years, 5 months ago (2016-07-06 20:17:06 UTC) #7
Lei Zhang
Does std::abs() work without casting?
4 years, 5 months ago (2016-07-06 22:47:04 UTC) #8
Adam Goode
On 2016/07/06 22:47:04, Lei Zhang wrote: > Does std::abs() work without casting? No. I could ...
4 years, 5 months ago (2016-07-07 00:01:45 UTC) #9
Lei Zhang
Can you better explain the build environment? Are you trying to build Chromium on F24 ...
4 years, 5 months ago (2016-07-07 00:06:25 UTC) #10
Lei Zhang
Oh, on Ubuntu, if I switch to std::abs(), then I hit the same error.
4 years, 5 months ago (2016-07-07 00:08:51 UTC) #11
Lei Zhang
Can you see if this works? std::abs(static_cast<int>(dt.tzHour))
4 years, 5 months ago (2016-07-07 00:19:33 UTC) #12
Adam Goode
On 2016/07/07 00:19:33, Lei Zhang wrote: > Can you see if this works? > > ...
4 years, 5 months ago (2016-07-07 00:31:34 UTC) #13
Lei Zhang
On 2016/07/07 00:31:34, Adam Goode wrote: > On 2016/07/07 00:19:33, Lei Zhang wrote: > > ...
4 years, 5 months ago (2016-07-07 00:35:52 UTC) #15
Adam Goode
On 2016/07/07 00:08:51, Lei Zhang wrote: > Oh, on Ubuntu, if I switch to std::abs(), ...
4 years, 5 months ago (2016-07-07 00:36:59 UTC) #16
Lei Zhang
On 2016/07/07 00:36:59, Adam Goode wrote: > On 2016/07/07 00:08:51, Lei Zhang wrote: > > ...
4 years, 5 months ago (2016-07-07 00:41:11 UTC) #17
Lei Zhang
So how about std::abs() and change line 292 to be consistent, and call this done?
4 years, 5 months ago (2016-07-07 01:10:24 UTC) #18
Adam Goode
On 2016/07/07 01:10:24, Lei Zhang wrote: > So how about std::abs() and change line 292 ...
4 years, 5 months ago (2016-07-07 01:16:35 UTC) #19
Lei Zhang
lgtm
4 years, 5 months ago (2016-07-07 01:19:45 UTC) #20
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2124083002/40001
4 years, 5 months ago (2016-07-07 01:36:55 UTC) #22
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-07-07 01:53:26 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2124083002/40001
4 years, 5 months ago (2016-07-07 01:57:05 UTC) #26
commit-bot: I haz the power
4 years, 5 months ago (2016-07-07 07:38:39 UTC) #28
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/c9a0ec50d813d13498830c05d60d01360729...

Powered by Google App Engine
This is Rietveld 408576698