|
|
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. |
DescriptionFix 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 #Messages
Total messages: 28 (8 generated)
agoode@chromium.org changed reviewers: + thestig@chromium.org
I don't seem to be able to trigger any trybots as I would with chromium. $ git cl try Loaded default bots from CQ config (/home/agoode/pdfium/pdfium/infra/config/cq.cfg) ERROR: Access denied: User user:agoode@chromium.org cannot add builds to bucket master.tryserver.client.pdfium
On 2016/07/06 19:59:29, Adam Goode wrote: > I don't seem to be able to trigger any trybots as I would with chromium. > > $ git cl try > Loaded default bots from CQ config > (/home/agoode/pdfium/pdfium/infra/config/cq.cfg) > ERROR: Access denied: User user:agoode@chromium.org cannot add builds to bucket > master.tryserver.client.pdfium The 'CQ dry run' link should do what you want I believe. The trybot list is a separate list from the chromium one.
The CQ bit was checked by agoode@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
Does std::abs() work without casting?
On 2016/07/06 22:47:04, Lei Zhang wrote: > Does std::abs() work without casting? No. I could spend some time untangling the headers if you like, but it looks like a mess.
Can you better explain the build environment? Are you trying to build Chromium on F24 and failing in PDFium? For whatever reason, Ninja likes to build PDFium first in a Chromium build. Is the build configuration g++ and no sysroot? Can I install F24 and reproduce the problem with a standalone PDFium checkout? https://codereview.chromium.org/2124083002/diff/1/fpdfsdk/fsdk_baseannot.cpp File fpdfsdk/fsdk_baseannot.cpp (right): https://codereview.chromium.org/2124083002/diff/1/fpdfsdk/fsdk_baseannot.cpp#... fpdfsdk/fsdk_baseannot.cpp:292: str2.Format("%02d:%02u", abs(dt.tzHour), dt.tzMinute); Also, not complaining here?
Oh, on Ubuntu, if I switch to std::abs(), then I hit the same error.
Can you see if this works? std::abs(static_cast<int>(dt.tzHour))
On 2016/07/07 00:19:33, Lei Zhang wrote: > Can you see if this works? > > std::abs(static_cast<int>(dt.tzHour)) Both of these work: abs(static_cast<int>(dt.tzHour)) std::abs(static_cast<int>(dt.tzHour)) Do you want std::abs or abs? My vote is abs to match what was there. It definitely seems better to do the static_cast inside.
Description was changed from ========== 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] BUG= ========== to ========== 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] ==========
On 2016/07/07 00:31:34, Adam Goode wrote: > On 2016/07/07 00:19:33, Lei Zhang wrote: > > Can you see if this works? > > > > std::abs(static_cast<int>(dt.tzHour)) > > Both of these work: > abs(static_cast<int>(dt.tzHour)) > std::abs(static_cast<int>(dt.tzHour)) > > Do you want std::abs or abs? My vote is abs to match what was there. In Chromium: git grep ' abs('|grep cc:|wc -l -> 60 git grep 'std::abs('|grep cc:|wc -l -> 314 ... so std::abs? > It definitely seems better to do the static_cast inside. Agreed.
On 2016/07/07 00:08:51, Lei Zhang wrote: > Oh, on Ubuntu, if I switch to std::abs(), then I hit the same error. To answer your other questions, this was only hit while building chromium on Fedora with sysroot=0. Standalone pdfium builds fine as is.
On 2016/07/07 00:36:59, Adam Goode wrote: > On 2016/07/07 00:08:51, Lei Zhang wrote: > > Oh, on Ubuntu, if I switch to std::abs(), then I hit the same error. > > To answer your other questions, this was only hit while building chromium on > Fedora with sysroot=0. Standalone pdfium builds fine as is. Standalone PDFium also picked up Chromium's sysroot=1 default setting and I haven't gotten around to turning it off. F24 has a newer version of g++ and the headers have changed. We ran into a std::isnan() error earlier with g++ 5.
So how about std::abs() and change line 292 to be consistent, and call this done?
On 2016/07/07 01:10:24, Lei Zhang wrote: > So how about std::abs() and change line 292 to be consistent, and call this > done? Sounds good.
lgtm
The CQ bit was checked by agoode@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by agoode@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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] ========== to ========== 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/+/c9a0ec50d813d13498830c05d60d01360729... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/c9a0ec50d813d13498830c05d60d01360729... |